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

Latest patch looks good to me.

Regards,
Rushabh

On Wed, Jun 26, 2013 at 11:21 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>wrote:

> Hello
>
> updated patch with some basic doc
>
> Regards
>
> Pavel
>
>
> 2013/6/26 Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>:
> >
> >
> >
> > On Wed, Jun 26, 2013 at 5:11 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> > wrote:
> >>
> >> 2013/6/26 Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>:
> >> >
> >> >
> >> >
> >> > On Tue, Jun 25, 2013 at 11:09 AM, Pavel Stehule
> >> > <pavel(dot)stehule(at)gmail(dot)com>
> >> > wrote:
> >> >>
> >> >> 2013/6/25 Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>:
> >> >> >
> >> >> >
> >> >> >
> >> >> > On Tue, Jun 25, 2013 at 1:47 AM, Pavel Stehule
> >> >> > <pavel(dot)stehule(at)gmail(dot)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(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.
> >> >> >
> >> >> >
> >> >> > 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(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
> >> >> >
> >> >> >
> >> >> >
> >> >> >
> >> >> > --
> >> >> > Rushabh Lathia
> >> >
> >> >
> >> >
> >> >
> >> > --
> >> > Rushabh Lathia
> >
> >
> >
> >
> > --
> > Rushabh Lathia
>

--
Rushabh Lathia