Re: [PATCH] add concurrent_abort callback for output plugin

Hi,

On 2021-03-25 10:07:28 +0100, Markus Wanner wrote:
> This leads to the possibility of the transaction getting aborted concurrent
> to logical decoding. In that case, it is likely for the decoder to error on
> a catalog scan that conflicts with the abort of the transaction. The
> reorderbuffer sports a PG_CATCH block to cleanup.

FWIW, I am seriously suspicuous of the code added as part of
7259736a6e5b7c75 and plan to look at it after the code freeze. I can't
really see this code surviving as is. The tableam.h changes, the bsyscan
stuff, ... Leaving correctness aside, the code bloat and performance
affects alone seems problematic.

> Again, this callback is especially important for output plugins that invoke
> further actions on downstream nodes that delay the COMMIT PREPARED of a
> transaction upstream, e.g. until prepared on other nodes. Up until now, the
> output plugin has no way to learn about a concurrent abort of the currently
> decoded (2PC or streamed) transaction (perhaps short of continued polling on
> the transaction status).

You may have only meant it as a shorthand: But imo output plugins have
absolutely no business "invoking actions downstream".

> diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
> index c291b05a423..a6d044b870b 100644
> --- a/src/backend/replication/logical/reorderbuffer.c
> +++ b/src/backend/replication/logical/reorderbuffer.c
> @@ -2488,6 +2488,12 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
> errdata = NULL;
> curtxn->concurrent_abort = true;
>
> + /*
> + * Call the cleanup hook to inform the output plugin that the
> + * transaction just started had to be aborted.
> + */
> + rb->concurrent_abort(rb, txn, streaming, commit_lsn);
> +
> /* Reset the TXN so that it is allowed to stream remaining data. */
> ReorderBufferResetTXN(rb, txn, snapshot_now,
> command_id, prev_lsn,

I don't think this would be ok, errors thrown in the callback wouldn't
be handled as they would be in other callbacks.

Greetings,

Andres Freund