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

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