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