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

2013/6/25 Rushabh Lathia <rushabh.lathia@gmail.com>:
>
>
>
> On Tue, Jun 25, 2013 at 1:47 AM, Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
>>
>> 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.
>
>
> Hmm ok.
>
>>
>>
>> >
>> >
>> > 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();
>> }
>>
>> ???
>
>
> Nope. rather then that I would still prefer direct call of
> FlushErrorState().
>
>>
>>
>> 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 don't know but I still feel like given header from function it self would
> be
> nice to have things. Because it will help user to differentiate between
> STACK and normal NOTICE context message.
>
>
>>
>>
>> >
>> > 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,
>
>
> Ok
>>
>>
>> *2 it is possible - I have not strong opinion
>
>
> Prefer  FlushErrorState() call.
>

ok

>>
>> *3 no, I would to return data in most simply and clean form without any
>> sugar
>
>
> As a user perspective it would be nice to have sugar layer around.
>

I understand, but disagree - strings are simply joined and is
difficult to separate it. I have strong opinion here.

* a reason for this construct is not only using in debug notices
* sugar is not used in GET DIAGNOSTICS statement ever - so possible
introduction is not consistent with other

Regards

Pavel


>>
>>
>> What do you think?
>
>
> Others or committer having any opinion ?
>

???

>>
>>
>> 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
>
>
>
>
> --
> Rushabh Lathia