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 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers