Re: pgsql: Add GET DIAGNOSTICS ... PG_CONTEXT in PL/PgSQL

Add GET DIAGNOSTICS ... PG_CONTEXT in PL/PgSQL

This adds the ability to get the call stack as a string from within a
PL/PgSQL function, which can be handy for logging to a table, or to
include in a useful message to an end-user.

Pavel Stehule, reviewed by Rushabh Lathia and rather heavily whacked
around by Stephen Frost.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/831283256796d1c20858862b568d73e505eb4a84

Modified Files
--------------
doc/src/sgml/plpgsql.sgml | 57 +++++++++++++++++++++++++++
src/backend/utils/error/elog.c | 69 +++++++++++++++++++++++++++++++++
src/include/utils/elog.h | 2 +
src/pl/plpgsql/src/pl_exec.c | 10 +++++
src/pl/plpgsql/src/pl_funcs.c | 2 +
src/pl/plpgsql/src/pl_gram.y | 6 +++
src/pl/plpgsql/src/pl_scanner.c | 1 +
src/pl/plpgsql/src/plpgsql.h | 1 +
src/test/regress/expected/plpgsql.out | 48 +++++++++++++++++++++++
src/test/regress/sql/plpgsql.sql | 33 ++++++++++++++++
10 files changed, 229 insertions(+)

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> Add GET DIAGNOSTICS ... PG_CONTEXT in PL/PgSQL

I don't find it to be a terribly good idea that GetErrorContextStack
does FlushErrorState(). Doesn't that imply that it can't safely be
used from inside error processing, which is more or less exactly
where it is intended to be used? I would certainly think it surprising
that that call destroys all information about the error.

For the same reason, it's rather dubious that it uses ErrorContext as
working space. There might not be a heck of a lot of space left there,
and I don't think that construction of this string really counts as
error processing. It seems to me to be something done outside the error
subsystem.

regards, tom lane

Tom,

On Wednesday, July 24, 2013, Tom Lane wrote:

> Stephen Frost <sfrost(at)snowman(dot)net <javascript:;>> writes:
> > Add GET DIAGNOSTICS ... PG_CONTEXT in PL/PgSQL
>
> I don't find it to be a terribly good idea that GetErrorContextStack
> does FlushErrorState(). Doesn't that imply that it can't safely be
> used from inside error processing, which is more or less exactly
> where it is intended to be used? I would certainly think it surprising
> that that call destroys all information about the error.

It's not intended (nor allowed, if I got it right) in an error context. I
will admit that it's not a wonderful situation to have it using the error
handling's internal components like this, but that's also where it has to
go for the callbacks to get the information needed.

> For the same reason, it's rather dubious that it uses ErrorContext as
> working space. There might not be a heck of a lot of space left there,
> and I don't think that construction of this string really counts as
> error processing. It seems to me to be something done outside the error
> subsystem.
>

My concerns and thoughts around this were what may happen if a callback
throws an error and it still makes me a bit nervous but It seems like it
should work. Still, if there's a reasonable way to collect the stack info
without going through the error callbacks, without implementing a duplicate
system, I'm all ears.

Thanks!

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> On Wednesday, July 24, 2013, Tom Lane wrote:
>> I don't find it to be a terribly good idea that GetErrorContextStack
>> does FlushErrorState().

> It's not intended (nor allowed, if I got it right) in an error context. I
> will admit that it's not a wonderful situation to have it using the error
> handling's internal components like this, but that's also where it has to
> go for the callbacks to get the information needed.

Sure. What I'm complaining about is that the function has side-effects
on the state of the error handler, which is surprising, undocumented,
and IMHO unsafe.

I think it should do its work in the caller's context, never mind
whether the errcontext callbacks leak a bit of memory; and it should
absolutely NOT be calling FlushErrorState afterwards. That should be
done by the caller when it's done with error processing.

> My concerns and thoughts around this were what may happen if a callback
> throws an error and it still makes me a bit nervous but It seems like it
> should work.

Right. In particular, what if we run out of memory while trying to
build the context string? That's not going to be a good situation in
any case, but I think this design turns it into a worst-case scenario.
It'd be better to be building the string outside the error handler's
reserved space.

regards, tom lane

On Wednesday, July 24, 2013, Stephen Frost wrote:

> Still, if there's a reasonable way to collect the stack info without going
> through the error callbacks, without implementing a duplicate system, I'm
> all ears.
>

That said, I'll work on making this more independent of the error handling
and see if it can be made to use an independent memory context and try to
tighten it up to ensure it isn't called in an error case. Future callers
may try to.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> That said, I'll work on making this more independent of the error handling
> and see if it can be made to use an independent memory context and try to
> tighten it up to ensure it isn't called in an error case. Future callers
> may try to.

I'm not following your reasoning here. This *has* to be called in an
error case, before you're outside the error processing context.
Otherwise there would be no data available to be printed.

In short: FlushErrorState, by definition, destroys the information that
GetErrorContextStack looks at. So in the current implementation,
GetErrorContextStack is burning its bridges behind it. That's at the
very least a surprising behavior. I am betting that it will have
unpleasant consequences for any sort of nested-error scenario.

regards, tom lane

On Wednesday, July 24, 2013, Tom Lane wrote:

> Stephen Frost <sfrost(at)snowman(dot)net <javascript:;>> writes:
> > That said, I'll work on making this more independent of the error
> handling
> > and see if it can be made to use an independent memory context and try to
> > tighten it up to ensure it isn't called in an error case. Future callers
> > may try to.
>
> I'm not following your reasoning here. This *has* to be called in an
> error case, before you're outside the error processing context.
> Otherwise there would be no data available to be printed.

It has to be called after the context callbacks have been set up by the
levels above it on the stack (eg: SPI calls), but that's not the same as
being called from an actual exception. The idea here is to simply execute
the callbacks for each level above it on the stack and collect the strings
they create with errcontext().

In short: FlushErrorState, by definition, destroys the information that
> GetErrorContextStack looks at. So in the current implementation,
> GetErrorContextStack is burning its bridges behind it. That's at the
> very least a surprising behavior. I am betting that it will have
> unpleasant consequences for any sort of nested-error scenario.
>

Unfortunately, I don't have the code in front of me at the moment, but I
was pretty sure FlushErrorState cleans up the intermediate information set
up during an individual error call and doesn't completely clear the stack
info (tho I can't think of what, if anything, does..) or multiple "raise
notices" wouldn't each contain the stack info in their output..

I'll certainly look through this again and dream up some additional test
cases for it, in the morning.

Thanks,

Stephen

On Wednesday, July 24, 2013, Stephen Frost wrote:

> Unfortunately, I don't have the code in front of me at the moment, but I
> was pretty sure FlushErrorState cleans up the intermediate information set
> up during an individual error call and doesn't completely clear the stack
> info (tho I can't think of what, if anything, does..) or multiple "raise
> notices" wouldn't each contain the stack info in their output..
>
> I'll certainly look through this again and dream up some additional test
> cases for it, in the morning.
>

Couldn't help if- reading code on a phone isn't ideal, but that's a
different discussion.

You're right- resetting the stack depth doesn't make any sense here, which
FlushErrorState does. I think clearing the memory context should be
alright but that's a different story. Curious that the raise notice in the
regression test didn't blow up, which is part of why I thought it was fine,
but a subsequent call to raise notice in the same function would be a good
test (along with another call to this function..) and I think wouldn't
produce a stack trace here, which would be quite wrong.

Will address once I'm back I front of something I can actually write code
on.

Thanks,

Stephen

On Thursday, July 25, 2013, Stephen Frost wrote:

> On Wednesday, July 24, 2013, Stephen Frost wrote:
>
>> Unfortunately, I don't have the code in front of me at the moment, but I
>> was pretty sure FlushErrorState cleans up the intermediate information set
>> up during an individual error call and doesn't completely clear the stack
>> info (tho I can't think of what, if anything, does..) or multiple "raise
>> notices" wouldn't each contain the stack info in their output..
>>
>> I'll certainly look through this again and dream up some additional test
>> cases for it, in the morning.
>>
>
> Couldn't help if- reading code on a phone isn't ideal, but that's a
> different discussion.
>
> You're right- resetting the stack depth doesn't make any sense here, which
> FlushErrorState does. I think clearing the memory context should be
> alright but that's a different story. Curious that the raise notice in the
> regression test didn't blow up, which is part of why I thought it was fine,
> but a subsequent call to raise notice in the same function would be a good
> test (along with another call to this function..) and I think wouldn't
> produce a stack trace here, which would be quite wrong.
>
> Will address once I'm back I front of something I can actually write code
> on.
>

Alright, no, and clearly I should have run this down before committing it,
but I knew the raise notice after the call to get diag meant we must not be
completely destroying the stack.

The errdata stack is different from the context stack info. errdata is for
reentrant error calls, which this function should never be a part of. Were
that to happen, at least the Assert around the recursion check should fire.
As for if we *should* call FlushErrorState, I think we need to, to reset
ErrorContext and move the errdata stack depth back to -1, where it should
be. Now, we might just forgo adding ourselves onto the stack, as we don't
set a callback anyway, but the errdata stack depth should certainly be at
-1 when we leave, or we haven't prevented reentrant calls into
GetErrorStack.

This definitely deserves more commentary and I'll see about adding that
also.

Thanks for reviewing!

Stephen

Tom,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> I'm not following your reasoning here. This *has* to be called in an
> error case, before you're outside the error processing context.
> Otherwise there would be no data available to be printed.
>
> In short: FlushErrorState, by definition, destroys the information that
> GetErrorContextStack looks at. So in the current implementation,
> GetErrorContextStack is burning its bridges behind it. That's at the
> very least a surprising behavior. I am betting that it will have
> unpleasant consequences for any sort of nested-error scenario.

I've just pushed up some much needed improvements to the comments which
hopefully clarify that this function is using error_context_stack and
calling the callbacks set up there by callers above on the PG call
stack. Also, hopefully this makes it clear that errrordata is required
to be empty when this function is called and therefore it can be cleaned
up when exiting with FlushErrorState.

Perhaps it would be better to try and work out a way for this to be
reentrant safe and be callable from an exception handler, but it
certainly wasn't part of the original intent and being able to support
either being called under an exception handler or not would essentially
require checking if anything is above us on the stack and, if not,
cleaning things up anyway, or trusting the above caller to handle it and
skipping it.

I'm happy to rework this or even revert it if this use of the
error_context_stack is just too grotty, but this certainly looks like a
useful capability to have.

Thanks!

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> I've just pushed up some much needed improvements to the comments which
> hopefully clarify that this function is using error_context_stack and
> calling the callbacks set up there by callers above on the PG call
> stack.

Now that I'm a bit more awake, I realize that my comments last night
were off-target. As you say, the purpose of this function is to extract
the context information that's been stacked against the possibility of a
future error, so it's unrelated to actual error processing, and the
FlushErrorState call is *not* destroying its input data as I claimed.

> Also, hopefully this makes it clear that errrordata is required
> to be empty when this function is called and therefore it can be cleaned
> up when exiting with FlushErrorState.

But having said that, "unrelated" does not mean "cannot be used together
with". I think this implementation of the function is dangerous (PANIC?
really?), overly restrictive, and overcomplicated to boot. The only
reason you need to call FlushErrorState is to get rid of any palloc's
leaked by errcontext functions, and the only reason that that's even
of concern is that you're allocating them in ErrorContext which is a
precious resource. I don't think this function has any business
touching ErrorContext at all, precisely because it isn't part of error
recovery. Indeed, by eating up reserved ErrorContext space, you
increase the risk of an error within this function being unrecoverable.
Saner would be either:

1. Just run the whole business in the caller's context and leave it up
to the caller to worry about whether it needs to clean up memory usage.

2. Create a temporary context underneath CurrentMemoryContext, use that,
and then delete it when done.

The only thing that needs to be considered an error condition is if the
errordata stack is too full to allow creation of the extra entry needed
by this function, which is an improbable situation. Other than
temporarily stacking and then unstacking that entry, you don't need to
have any side-effects on the state of the error subsystem.

> I'm happy to rework this or even revert it if this use of the
> error_context_stack is just too grotty, but this certainly looks like a
> useful capability to have.

I'm not objecting to the functionality, just to the implementation:
I think you could decouple this from error handling a lot better.

One other minor gripe is that the function is documented as returning
the "call stack", which a C programmer would think means something
entirely different from what is actually output. I think you need to
use a different phrase there. "error context stack" would be OK.

regards, tom lane

Hello

2013/7/25 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
>> I've just pushed up some much needed improvements to the comments which
>> hopefully clarify that this function is using error_context_stack and
>> calling the callbacks set up there by callers above on the PG call
>> stack.
>
> Now that I'm a bit more awake, I realize that my comments last night
> were off-target. As you say, the purpose of this function is to extract
> the context information that's been stacked against the possibility of a
> future error, so it's unrelated to actual error processing, and the
> FlushErrorState call is *not* destroying its input data as I claimed.
>
>> Also, hopefully this makes it clear that errrordata is required
>> to be empty when this function is called and therefore it can be cleaned
>> up when exiting with FlushErrorState.
>
> But having said that, "unrelated" does not mean "cannot be used together
> with". I think this implementation of the function is dangerous (PANIC?
> really?), overly restrictive, and overcomplicated to boot. The only
> reason you need to call FlushErrorState is to get rid of any palloc's
> leaked by errcontext functions, and the only reason that that's even
> of concern is that you're allocating them in ErrorContext which is a
> precious resource. I don't think this function has any business
> touching ErrorContext at all, precisely because it isn't part of error
> recovery. Indeed, by eating up reserved ErrorContext space, you
> increase the risk of an error within this function being unrecoverable.
> Saner would be either:
>
> 1. Just run the whole business in the caller's context and leave it up
> to the caller to worry about whether it needs to clean up memory usage.
>
> 2. Create a temporary context underneath CurrentMemoryContext, use that,
> and then delete it when done.
>
> The only thing that needs to be considered an error condition is if the
> errordata stack is too full to allow creation of the extra entry needed
> by this function, which is an improbable situation. Other than
> temporarily stacking and then unstacking that entry, you don't need to
> have any side-effects on the state of the error subsystem.
>
>> I'm happy to rework this or even revert it if this use of the
>> error_context_stack is just too grotty, but this certainly looks like a
>> useful capability to have.
>
> I'm not objecting to the functionality, just to the implementation:
> I think you could decouple this from error handling a lot better.
>
> One other minor gripe is that the function is documented as returning
> the "call stack", which a C programmer would think means something
> entirely different from what is actually output. I think you need to
> use a different phrase there. "error context stack" would be OK.

I used a ErrorContext because I wasn't sure so errcontext and similar
function can work in different context. Now I look there and there
should be well initialized ErrorDataStack, due

int
set_errcontext_domain(const char *domain)
{
<------>ErrorData *edata = &errordata[errordata_stack_depth];

<------>/* we don't bother incrementing recursion_depth */
<------>CHECK_STACK_DEPTH();

<------>edata->context_domain = domain;

<------>return 0;
}

but MemoryContext can be any - so probably some private context is ideal.

Regards

Pavel

>
> regards, tom lane
>
>
> --
> Sent via pgsql-committers mailing list (pgsql-committers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-committers

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> 1. Just run the whole business in the caller's context and leave it up
> to the caller to worry about whether it needs to clean up memory usage.

I'd certainly be fine with that, and had considered it, but it looks
like errcontext_msg() (which is called by the callbacks through the
errcontext() macro) switches to ErrorContext for its work, meaning we
have to clean up that context anyway. In my initial review I hadn't
considered the possibility of modifying what ErrorContext is pointing
to. As the error system may end up getting called by a callback
function, it seems like changing the global ErrorContext would be a bad
idea. Forgive me if I'm missing something obvious in how we could
simply use the caller's context for this as I'd surely be much happier
with that.

> 2. Create a temporary context underneath CurrentMemoryContext, use that,
> and then delete it when done.

That'd be fine with me also, but runs the same issue as above.

> The only thing that needs to be considered an error condition is if the
> errordata stack is too full to allow creation of the extra entry needed
> by this function, which is an improbable situation. Other than
> temporarily stacking and then unstacking that entry, you don't need to
> have any side-effects on the state of the error subsystem.

If we can address the memory context issue in a simple way then I'll
certainly set this up to be reentrant safe and to handle pushing and
popping itself on the errordata stack.

> One other minor gripe is that the function is documented as returning
> the "call stack", which a C programmer would think means something
> entirely different from what is actually output. I think you need to
> use a different phrase there. "error context stack" would be OK.

Works for me. I had already tried to reword it to address that exact
issue, but "error context stack" does sound better than "PG call stack."

Thanks!

Stephen

Pavel,

First, please only quote the relevant parts of the email when
responding.

* Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
> I used a ErrorContext because I wasn't sure so errcontext and similar
> function can work in different context. Now I look there and there
> should be well initialized ErrorDataStack, due
>
> int
> set_errcontext_domain(const char *domain)
> {
> <------>ErrorData *edata = &errordata[errordata_stack_depth];
>
> <------>/* we don't bother incrementing recursion_depth */
> <------>CHECK_STACK_DEPTH();
>
> <------>edata->context_domain = domain;
>
> <------>return 0;
> }
>
> but MemoryContext can be any - so probably some private context is ideal.

While set_errcontext_domain() doesn't care about the MemoryContext, per
se, the errcontext() macro further calls errcontext_msg() which is
currently set up to explicitly use ErrorContext. Perhaps an elog.c
global to tell errcontext_msg() to not switch memory contexts, but what
happens if there's an error thrown by a callback function..?

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> 1. Just run the whole business in the caller's context and leave it up
>> to the caller to worry about whether it needs to clean up memory usage.

> I'd certainly be fine with that, and had considered it, but it looks
> like errcontext_msg() (which is called by the callbacks through the
> errcontext() macro) switches to ErrorContext for its work, meaning we
> have to clean up that context anyway.

Yeah, I was just noticing that. We'd have to change that ...

> In my initial review I hadn't
> considered the possibility of modifying what ErrorContext is pointing
> to. As the error system may end up getting called by a callback
> function, it seems like changing the global ErrorContext would be a bad
> idea.

Yeah, that'd be a nonstarter. I thought about simply not doing
MemoryContextSwitchTo in errcontext_msg(), which would work for ordinary
usage in error context callbacks, because those are run in ErrorContext
anyway. However, there are some call sites that use errcontext() like a
regular ereport helper, for instance in dblink, and we'd end up with the
context string in the wrong memory context in that case. What seems
like a better idea is to add a memory-context-to-use field to struct
ErrorData. We'd initialize it to ErrorContext when pushing a stack
entry for normal error processing, but GetErrorContextStack could
do something different. This would eliminate most of the explicit
references to ErrorContext in elog.c.

If you want I'll draft something up.

regards, tom lane

2013/7/25 Stephen Frost <sfrost(at)snowman(dot)net>:
> Pavel,
>
> First, please only quote the relevant parts of the email when
> responding.
>
> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
>> I used a ErrorContext because I wasn't sure so errcontext and similar
>> function can work in different context. Now I look there and there
>> should be well initialized ErrorDataStack, due
>>
>> int
>> set_errcontext_domain(const char *domain)
>> {
>> <------>ErrorData *edata = &errordata[errordata_stack_depth];
>>
>> <------>/* we don't bother incrementing recursion_depth */
>> <------>CHECK_STACK_DEPTH();
>>
>> <------>edata->context_domain = domain;
>>
>> <------>return 0;
>> }
>>
>> but MemoryContext can be any - so probably some private context is ideal.
>
> While set_errcontext_domain() doesn't care about the MemoryContext, per
> se, the errcontext() macro further calls errcontext_msg() which is
> currently set up to explicitly use ErrorContext. Perhaps an elog.c
> global to tell errcontext_msg() to not switch memory contexts, but what
> happens if there's an error thrown by a callback function..?

Probably then will be raised a panic due recursion call of error API

Our PL doesn't raise a exception in callback function explicitly.
Probably there is risk - "out of memory" - but same risk is when you
call errcontext inside elog function.

Pavel

>
> Thanks,
>
> Stephen

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> [...] a better idea is to add a memory-context-to-use field to struct
> ErrorData. We'd initialize it to ErrorContext when pushing a stack
> entry for normal error processing, but GetErrorContextStack could
> do something different. This would eliminate most of the explicit
> references to ErrorContext in elog.c.

Ah, fantastic idea.

> If you want I'll draft something up.

I'll take care of it, thanks for the idea!

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> If you want I'll draft something up.

> I'll take care of it, thanks for the idea!

OK. One possibly non-obvious point is that I think the field should be
defined as "context containing associated non-constant strings"; this
would mean in particular that CopyErrorData would need to change it
to CurrentMemoryContext in the copied struct, and then ReThrowError
would change it back when restoring the data onto the error stack.
This detail is probably a no-op in current usages, but in the future it
might allow modification of a copied ErrorData while it's outside
ErrorContext, if anyone should want to do that.

Also I'd advise declaring the field as "struct MemoryContextData *"
to avoid having to include palloc.h into elog.h.

regards, tom lane

Tom,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> OK. One possibly non-obvious point is that I think the field should be
> defined as "context containing associated non-constant strings"; this
> would mean in particular that CopyErrorData would need to change it
> to CurrentMemoryContext in the copied struct, and then ReThrowError
> would change it back when restoring the data onto the error stack.
> This detail is probably a no-op in current usages, but in the future it
> might allow modification of a copied ErrorData while it's outside
> ErrorContext, if anyone should want to do that.
>
> Also I'd advise declaring the field as "struct MemoryContextData *"
> to avoid having to include palloc.h into elog.h.

Good points, all. Apologies for it taking a bit to get to this, but
please take a look when you get a chance. Barring objections, this is
what I'm planning to commit, which moves most calls to use the new
edata->alloc_context instead of ErrorContext. This also means we can
allow GET DIAG ... PG_CONTEXT to be called from exception handlers,
which I've set up and added regression tests for.

Thanks!

Stephen