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

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

On 2/2/13 3:23 AM, Pavel Stehule 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

That could *really* stand for some indentation in the output instead of the *** business. But other than that, this definitely seems useful. I had no idea _context was even an option... :/

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:

1) I noticed that you did the initialization of edata manually, just
wondering
can't we directly use CopyErrorData() which will do that job ?

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() ?

2) To reset stack to empty, FlushErrorState() can be used.

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 worked on point 2) and 3) and PFA patch for reference.

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

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.

>
>
> 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();
}

???

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 worked on point 2) and 3) and PFA patch for reference.

so

*1 CopyErrorData does one useless palloc more and it is too generic,

*2 it is possible - I have not strong opinion

*3 no, I would to return data in most simply and clean form without any sugar

What do you think?

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

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

2013/6/25 Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>:
>
>
>
> 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.
>

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

On Tue, Jun 25, 2013 at 11:09 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>wrote:

> 2013/6/25 Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>:
> >
> >
> >
> > 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.
> >
>
> 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(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
>

--
Rushabh Lathia

2013/6/26 Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>:
>
>
>
> On Tue, Jun 25, 2013 at 11:09 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
>>
>> 2013/6/25 Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>:
>> >
>> >
>> >
>> > 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.
>> >
>>
>> 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.

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(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
>
>
>
>
> --
> Rushabh Lathia

On Wed, Jun 26, 2013 at 5:11 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>wrote:

> 2013/6/26 Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>:
> >
> >
> >
> > On Tue, Jun 25, 2013 at 11:09 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com
> >
> > wrote:
> >>
> >> 2013/6/25 Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>:
> >> >
> >> >
> >> >
> >> > 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.
> >> >
> >>
> >> 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(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
> >
> >
> >
> >
> > --
> > Rushabh Lathia
>

--
Rushabh Lathia

Hello

updated patch with some basic doc

Regards

Pavel

2013/6/26 Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>:
>
>
>
> On Wed, Jun 26, 2013 at 5:11 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
>>
>> 2013/6/26 Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>:
>> >
>> >
>> >
>> > On Tue, Jun 25, 2013 at 11:09 AM, Pavel Stehule
>> > <pavel(dot)stehule(at)gmail(dot)com>
>> > wrote:
>> >>
>> >> 2013/6/25 Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>:
>> >> >
>> >> >
>> >> >
>> >> > 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.
>> >> >
>> >>
>> >> 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(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
>> >
>> >
>> >
>> >
>> > --
>> > Rushabh Lathia
>
>
>
>
> --
> Rushabh Lathia

Latest patch looks good to me.

Regards,
Rushabh

On Wed, Jun 26, 2013 at 11:21 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>wrote:

> Hello
>
> updated patch with some basic doc
>
> Regards
>
> Pavel
>
>
> 2013/6/26 Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>:
> >
> >
> >
> > On Wed, Jun 26, 2013 at 5:11 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> > wrote:
> >>
> >> 2013/6/26 Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>:
> >> >
> >> >
> >> >
> >> > On Tue, Jun 25, 2013 at 11:09 AM, Pavel Stehule
> >> > <pavel(dot)stehule(at)gmail(dot)com>
> >> > wrote:
> >> >>
> >> >> 2013/6/25 Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>:
> >> >> >
> >> >> >
> >> >> >
> >> >> > 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.
> >> >> >
> >> >>
> >> >> 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(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
> >> >
> >> >
> >> >
> >> >
> >> > --
> >> > Rushabh Lathia
> >
> >
> >
> >
> > --
> > Rushabh Lathia
>

--
Rushabh Lathia

All,

* Rushabh Lathia (rushabh(dot)lathia(at)gmail(dot)com) wrote:
> Latest patch looks good to me.

I've committed this, with some pretty serious clean-up. In the future,
please don't simply copy/paste code w/o any updates to the comments
included, particularly when the comments no longer make sense in the new
place.

Thanks,

Stephen

Hello

2013/7/25 Stephen Frost <sfrost(at)snowman(dot)net>:
> All,
>
> * Rushabh Lathia (rushabh(dot)lathia(at)gmail(dot)com) wrote:
>> Latest patch looks good to me.
>
> I've committed this, with some pretty serious clean-up. In the future,
> please don't simply copy/paste code w/o any updates to the comments
> included, particularly when the comments no longer make sense in the new
> place.

Thank you very much

Regards

Pavel

>
> Thanks,
>
> Stephen