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

Hi,

On 2017-08-30 01:27:34 +0200, Tomas Vondra wrote:
> 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.

That's not really a valid thing to do, you should put it after the
BeginInternalSubTransaction()/StartTransactionCommand(). It's basically
assumed that those won't fail - arguably they should be outside of a
PG_TRY then, but that's a different matter. If you start decoding
outside of SQL failing before those will lead to rolling back the parent
tx...

> 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.

So, IIUC, there's no live problem in postgres core, besides some ugly &
undocumented assumptions?

Greetings,

Andres Freund