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
