Re: [PATCH] add concurrent_abort callback for output plugin

On 25.03.21 21:21, Andres Freund wrote:
> ... the code added as part of 7259736a6e5b7c75 ...

That's the streaming part, which can be thought of as a more general
variant of the two-phase decoding in that it allows multiple "flush
points" (invoking ReorderBufferProcessTXN). Unlike the PREPARE of a
two-phase commit, where the reorderbuffer can be sure there's no further
change to be processed after the PREPARE. Nor is there any invocation
of ReorderBufferProcessTXN before that fist one at PREPARE time. With
that in mind, I'm surprised support for streaming got committed before
2PC. It clearly has different use cases, though.

However, I'm sure your inputs on how to improve and cleanup the
implementation will be appreciated. The single tiny problem this patch
addresses is the same for 2PC and streaming decoding: the output plugin
currently has no way to learn about a concurrent abort of a transaction
still being decoded, at the time this happens.

Both, 2PC and streaming do require the reorderbuffer to forward changes
(possibly) prior to the transaction's commit. That's the whole point of
these two features. Therefore, I don't think we can get around
concurrent aborts.

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

From my point of view, that's the raison d'être for an output plugin.
Even if it does so merely by forwarding messages. But yeah, of course a
whole bunch of other components and changes are needed to implement the
kind of global two-phase commit system I tried to describe.

I'm open to suggestions on how to reference that use case.

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

That's a good point. Maybe the CATCH block should only set a flag,
allowing for the callback to be invoked outside of it.

Regards

Markus my-callbacks-do-not-throw-error Wanner