Re: error-handling in ReorderBufferCommit() seems somewhat broken

On 08/30/2017 02:19 AM, Andres Freund wrote:
> On 2017-08-30 02:11:10 +0200, Tomas Vondra wrote:
>>
>> I'm not really following your reasoning. You may very well be right that
>> the BeginInternalSubTransaction() example is not quite valid on postgres
>> core, but I don't see how that implies there can be no other errors in
>> the PG_TRY block. It was merely an explanation how I noticed this issue.
>>
>> To make it absolutely clear, I claim that the PG_CATCH() block is
>> demonstrably broken as it calls AbortCurrentTransaction() and then
>> accesses already freed memory.
>
> Yea, but that's not what happens normally. The txn memory isn't
> allocated in the context created by the started [sub]transaction. I
> think you're just seeing what you're seeing because an error thrown
> before the BeginInternalSubTransaction() succeeds, will roll back the
> *containing* transaction (the one that did the SQL SELECT * FROM
> pg_*logical*() call), and free all the entire reorderbuffer stuff.
>
>
>> The switch in the PG_TRY() blocks includes multiple elog(ERROR) calls in
>> the switch, and AFAICS hitting any of them will have exactly the same
>> effect as failure in BeginInternalSubTransaction().
>
> No, absolutely not. Once the subtransaction starts, the
> AbortCurrentTransaction() will abort that subtransaction, rather than
> the containing one.
>

Ah, I see. You're right elog(ERROR) calls after the subtransaction start
are handled correctly and don't trigger a segfault. Sorry for the noise
and thanks for the explanation.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services