error-handling in ReorderBufferCommit() seems somewhat broken

Hi,

I've been investigating some failures in test_decoding regression tests,
and it seems to me the error-handling in ReorderBufferCommit() is
somewhat broken, leading to segfault crashes.

The problematic part is this:

PG_CATCH()
{
/*
* Force cache invalidation to happen outside of a valid transaction
* to prevent catalog access as we just caught an error.
*/
AbortCurrentTransaction();

/* make sure there's no cache pollution */
ReorderBufferExecuteInvalidations(rb, txn);

...

}

Triggering it trivial - just add elog(ERROR,...) at the beginning of the
PG_TRY() block.

The problem is that AbortCurrentTransaction() apparently releases the
memory where txn is allocated, making it entirely bogus. So in assert
builds txn->ivalidations are 0x7f7f7f7f7f7f7f7f, triggering a segfault.

Similar issues apply to subsequent calls in the catch block, that also
use txn in some way (e.g. through snapshot_now).

I suppose this is not quite intentional, but rather an example that
error-handling code is an order of magnitude more complicated to write
and test. I've only noticed as I'm investigating some regression
failures on Postgres-XL 10, which does not support subtransactions and
so the BeginInternalSubTransaction() call in the try branch always
fails, triggering the issue.

regards

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