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

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.

> *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.

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