On 21.02.21 03:04, Andres Freund wrote:
Cost-wise, yes - a 2pc prepare/commit is expensive enough that
comparatively the replay cost is unlikely to be relevant.
Good. I attached an updated patch eliminating only the filtering for
empty two-phase transactions.
Behaviourally I'm still not convinced it's useful.
I don't have any further argument than: If you're promising to replicate
two phases, I expect the first phase to be replicated individually.
A database state with a transaction prepared and identified by
'woohoo-roll-me-back-if-you-can' is not the same as a state without it.
Even if the transaction is empty, or if you're actually going to roll
it back. And therefore possibly ending up at the very same state without
any useful effect.
Regards
Markus
From: Markus Wanner <markus.wan...@enterprisedb.com>
Date: Sun, 21 Feb 2021 10:43:34 +0100
Subject: [PATCH] Present empty prepares to the output plugin
While clearly not making any substantial changes to the data, a
PREPARE of an empty transaction still carries a gid that acts as a
sentinel for a prepared transaction that may be committed or rolled
back.
Drop the filtering of empty prepared transactions on the Postgres
side. An output plugin can still filter empty prepares, if it
wishes to do so.
---
.../replication/logical/reorderbuffer.c | 45 ++++++++++++-------
1 file changed, 29 insertions(+), 16 deletions(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 5a62ab8bbc1..18195255007 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -2016,7 +2016,8 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
ReorderBufferBuildTupleCidHash(rb, txn);
/* setup the initial snapshot */
- SetupHistoricSnapshot(snapshot_now, txn->tuplecid_hash);
+ if (snapshot_now)
+ SetupHistoricSnapshot(snapshot_now, txn->tuplecid_hash);
/*
* Decoding needs access to syscaches et al., which in turn use
@@ -2289,6 +2290,8 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
break;
case REORDER_BUFFER_CHANGE_INTERNAL_SNAPSHOT:
+ Assert(snapshot_now);
+
/* get rid of the old */
TeardownHistoricSnapshot(false);
@@ -2321,6 +2324,7 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
break;
case REORDER_BUFFER_CHANGE_INTERNAL_COMMAND_ID:
+ Assert(snapshot_now);
Assert(change->data.command_id != InvalidCommandId);
if (command_id < change->data.command_id)
@@ -2397,8 +2401,11 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
* streaming mode.
*/
if (streaming)
+ {
+ Assert(snapshot_now);
ReorderBufferSaveTXNSnapshot(rb, txn, snapshot_now, command_id);
- else if (snapshot_now->copied)
+ }
+ else if (snapshot_now && snapshot_now->copied)
ReorderBufferFreeSnap(rb, snapshot_now);
/* cleanup */
@@ -2520,7 +2527,6 @@ ReorderBufferReplay(ReorderBufferTXN *txn,
TimestampTz commit_time,
RepOriginId origin_id, XLogRecPtr origin_lsn)
{
- Snapshot snapshot_now;
CommandId command_id = FirstCommandId;
txn->final_lsn = commit_lsn;
@@ -2547,25 +2553,32 @@ ReorderBufferReplay(ReorderBufferTXN *txn,
* If this transaction has no snapshot, it didn't make any changes to the
* database, so there's nothing to decode. Note that
* ReorderBufferCommitChild will have transferred any snapshots from
- * subtransactions if there were any.
+ * subtransactions if there were any. This effectively makes empty
+ * transactions invisible to the output plugin.
+ *
+ * An empty two-phase transaction must not be short-circuited in the
+ * same way, because it carries a gid which may carry useful
+ * information, even if it is only a sentinel for an uncommitted empty
+ * transaction.
*/
- if (txn->base_snapshot == NULL)
+ if (txn->base_snapshot == NULL && !rbtxn_prepared(txn))
{
Assert(txn->ninvalidations == 0);
-
- /*
- * Removing this txn before a commit might result in the computation
- * of an incorrect restart_lsn. See SnapBuildProcessRunningXacts.
- */
- if (!rbtxn_prepared(txn))
- ReorderBufferCleanupTXN(rb, txn);
+ ReorderBufferCleanupTXN(rb, txn);
return;
}
- snapshot_now = txn->base_snapshot;
-
- /* Process and send the changes to output plugin. */
- ReorderBufferProcessTXN(rb, txn, commit_lsn, snapshot_now,
+ /*
+ * Process and send the changes to output plugin.
+ *
+ * Note that for empty transactions, txn->base_snapshot may well be NULL
+ * in case of an empty two-phase transaction. The corresponding
+ * callbacks will still be invoked, as even an empty transaction carries
+ * information (LSN increments, the gid in case of a two-phase
+ * transaction). This is unlike versions prior to 13 which optimized
+ * away empty transactions.
+ */
+ ReorderBufferProcessTXN(rb, txn, commit_lsn, txn->base_snapshot,
command_id, false);
}
--
2.30.0
