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

2013/6/26 Rushabh Lathia <rushabh.lathia@gmail.com>:
>
>
>
> On Tue, Jun 25, 2013 at 11:09 AM, Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
>>
>> 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
>
>
> Hmm because sugar is not used in GET DIAGNOSTICS statement ever,
> I think its should be fine.
>
>
> How about point 2) To reset stack to empty, FlushErrorState() can be used. ?

I can use FlushErrorState() - final decision should do commiter.

Thank you

I'll send remastered patch today.

Regards

Pavel

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