Re: pgsql: Add GET DIAGNOSTICS ... PG_CONTEXT in PL/PgSQL - Mailing list pgsql-committers

Stephen Frost <sfrost@snowman.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