Re: proposal 9.4 plpgsql: allows access to call stack from GET DIAGNOSTICS statement

Hello

This is fragment ofmy old code from orafce package - it is functional,
but it is written little bit more generic due impossible access to
static variables (in elog.c)

static char*
dbms_utility_format_call_stack(char mode)
{
MemoryContext oldcontext = CurrentMemoryContext;
ErrorData *edata;
ErrorContextCallback *econtext;
StringInfo sinfo;

#if PG_VERSION_NUM >= 80400
errstart(ERROR, __FILE__, __LINE__, PG_FUNCNAME_MACRO, TEXTDOMAIN);
#else
errstart(ERROR, __FILE__, __LINE__, PG_FUNCNAME_MACRO);
#endif

MemoryContextSwitchTo(oldcontext);

for (econtext = error_context_stack;
econtext != NULL;
econtext = econtext->previous)
(*econtext->callback) (econtext->arg);

edata = CopyErrorData();
FlushErrorState();

https://github.com/orafce/orafce/blob/master/utility.c

2013/6/24 Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>:
> Hi,
>
> Use of this feature is to get call stack for debugging without raising
> exception. This definitely seems very useful.
>
> Here are my comments about the submitted patch:
>
> Patch gets applied cleanly (there are couple of white space warning but
> that's
> into test case file). make and make install too went smooth. make check
> was smooth too. Did some manual testing about feature and its went smooth.
>
> However would like to share couple of points:
>

My main motive is concentration to stack info string only. So I didn't
use err_start function, and I didn't use CopyErrorData too. These
routines does lot of other work.

> 1) I noticed that you did the initialization of edata manually, just
> wondering
> can't we directly use CopyErrorData() which will do that job ?
>

yes, we can, but in this context on "context" field is interesting for us.

> I found that inside CopyErrorData() there is Assert:
>
> Assert(CurrentMemoryContext != ErrorContext);
>
> And in the patch we are setting using ErrorContext as short living context,
> is
> it the reason of not using CopyErrorData() ?

it was not a primary reason, but sure - this routine is not designed
for direct using from elog module. Next, we can save one palloc call.

>
>
> 2) To reset stack to empty, FlushErrorState() can be used.
>

yes, it can be. I use explicit rows due different semantics, although
it does same things. FlushErrorState some global ErrorState and it is
used from other modules. I did a reset of ErrorContext. I fill a small
difference there - so FlushErrorState is not best name for my purpose.
What about modification:

static void
resetErrorStack()
{
errordata_stack_depth = -1;
recursion_depth = 0;
/* Delete all data in ErrorContext */
MemoryContextResetAndDeleteChildren(ErrorContext);
}

and then call in InvokeErrorCallbacks -- resetErrorStack()

and rewrote flushErrorState like

void FlushErrorState(void)
{
/* reset ErrorStack is enough */
resetErrorStack();
}

???

but I can live well with direct call of FlushErrorState() - it is only
minor change.

> 3) I was think about the usability of the feature and wondering how about if
> we add some header to the stack, so user can differentiate between STACK and
> normal NOTICE message ?
>
> Something like:
>
> postgres=# select outer_outer_func(10);
> NOTICE: ----- Call Stack -----
> PL/pgSQL function inner_func(integer) line 4 at GET DIAGNOSTICS
> PL/pgSQL function outer_func(integer) line 3 at RETURN
> PL/pgSQL function outer_outer_func(integer) line 3 at RETURN
> CONTEXT: PL/pgSQL function outer_func(integer) line 3 at RETURN
> PL/pgSQL function outer_outer_func(integer) line 3 at RETURN
> outer_outer_func
> ------------------
> 20
> (1 row)

I understand, but I don't think so it is good idea. Sometimes, you
would to use context for different purposes than debug log - for
example - you should to identify top most call or near call - and any
additional lines means some little bit more difficult parsing. You can
add this line simply (if you want)

RAISE NOTICE e'--- Call Stack ---\n%', stack

but there are more use cases, where you would not this information (or
you can use own header (different language)), and better returns only
clean data.

>
> I worked on point 2) and 3) and PFA patch for reference.

so

*1 CopyErrorData does one useless palloc more and it is too generic,

*2 it is possible - I have not strong opinion

*3 no, I would to return data in most simply and clean form without any sugar

What do you think?

Regards

Pavel

>
> Thanks,
> Rushabh
>
>
>
> On Sat, Feb 2, 2013 at 2:53 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
>>
>> Hello
>>
>> I propose enhancing GET DIAGNOSTICS statement about new field
>> PG_CONTEXT. It is similar to GET STACKED DIAGNOSTICS'
>> PG_EXCEPTION_CONTEXT.
>>
>> Motivation for this proposal is possibility to get call stack for
>> debugging without raising exception.
>>
>> This code is based on cleaned code from Orafce, where is used four
>> years without any error reports.
>>
>> CREATE OR REPLACE FUNCTION public."inner"(integer)
>> RETURNS integer
>> LANGUAGE plpgsql
>> AS $function$
>> declare _context text;
>> begin
>> get diagnostics _context = pg_context;
>> raise notice '***%***', _context;
>> return 2 * $1;
>> end;
>> $function$
>>
>> postgres=# select outer_outer(10);
>> NOTICE: ***PL/pgSQL function "inner"(integer) line 4 at GET DIAGNOSTICS
>> PL/pgSQL function "outer"(integer) line 3 at RETURN
>> PL/pgSQL function outer_outer(integer) line 3 at RETURN***
>> CONTEXT: PL/pgSQL function "outer"(integer) line 3 at RETURN
>> PL/pgSQL function outer_outer(integer) line 3 at RETURN
>> outer_outer
>> ─────────────
>> 20
>> (1 row)
>>
>> Ideas, comments?
>>
>> Regards
>>
>> Pavel Stehule
>>
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>>
>
>
>
> --
> Rushabh Lathia