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

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