On Tue, Jun 25, 2013 at 11:09 AM, Pavel Stehule <pavel.steh...@gmail.com>wrote:
> 2013/6/25 Rushabh Lathia <rushabh.lat...@gmail.com>:
> >
> >
> >
> > On Tue, Jun 25, 2013 at 1:47 AM, Pavel Stehule <pavel.steh...@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.lat...@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. ?
> 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.steh...@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
