Re: proposal 9.4 plpgsql: allows access to call stack from GET DIAGNOSTICS statement - Mailing list pgsql-hackers

Hello

updated patch with some basic doc

Regards

Pavel


2013/6/26 Rushabh Lathia <rushabh.lathia@gmail.com>:
>
>
>
> On Wed, Jun 26, 2013 at 5:11 PM, Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
>>
>> 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.
>
>
> Thanks Pavel.
>
>>
>>
>> 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
>
>
>
>
> --
> Rushabh Lathia