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

Hello

2013/7/25 Tom Lane <tgl@sss.pgh.pa.us>:
> 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.

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@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-committers