On Wed, Jun 26, 2013 at 5:11 PM, Pavel Stehule <pavel.steh...@gmail.com>wrote:
> 2013/6/26 Rushabh Lathia <rushabh.lat...@gmail.com>: > > > > > > > > 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. ? > > 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.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 > -- Rushabh Lathia