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.lathia@gmail.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.stehule@gmail.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@postgresql.org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>>
>
>
>
> --
> Rushabh Lathia