Hi,
Thanks for updating the patch.
My idea was to call the cleanup function only once. A cleanup function can
accept
a list of sub transactions, and checks them while iterating the tuplecids of the
top transaction. Per my understanding we can use the binary search because the
list
of sub transactions are sorted by the logical XID. Attached .txt implements the
idea.
> For cfbot, reattaching v5 for master only. The REL_* attachments in my
> previous message are alternative backpatch versions, not a patch
> series. Cfbot applied the master patch successfully, then tried to
> apply the REL_14_STABLE version on top of it and reported conflicts.
TIPS: attaching .txt is a known workaround [1].
[1]: https://wiki.postgresql.org/wiki/Cfbot
Best regards,
Hayato Kuroda
FUJITSU LIMITED
diff --git a/src/backend/replication/logical/decode.c
b/src/backend/replication/logical/decode.c
index 0e7d4840a8e..c45edae853a 100644
--- a/src/backend/replication/logical/decode.c
+++ b/src/backend/replication/logical/decode.c
@@ -895,9 +895,9 @@ DecodeAbort(LogicalDecodingContext *ctx, XLogRecordBuffer
*buf,
/*
* Remove tuplecid changes queued by the aborted
(sub)transactions
* from the toplevel's list, before the transactions are torn
down.
- * The abort record's primary xid tells
- * ReorderBufferCleanupSubTxnTupleCids() whether the whole
toplevel
- * is going away, in which case scanning the list is pointless.
+ * The abort record's primary xid tells the cleanup function
whether
+ * the whole toplevel is going away, in which case scanning the
list is
+ * pointless.
*
* Note that we must not try to decide that from the primary
xid's
* own association instead: an abort record is written once the
@@ -909,16 +909,16 @@ DecodeAbort(LogicalDecodingContext *ctx, XLogRecordBuffer
*buf,
* all, while the released inner subtransactions it rolls back
do
* have one and their tuplecids still need to be removed.
*/
+ ReorderBufferCleanupAbortedSubTxnTupleCids(ctx->reorder, xid,
+
parsed->nsubxacts,
+
parsed->subxacts);
+
for (i = 0; i < parsed->nsubxacts; i++)
{
- ReorderBufferCleanupSubTxnTupleCids(ctx->reorder,
-
parsed->subxacts[i],
-
xid);
ReorderBufferAbort(ctx->reorder, parsed->subxacts[i],
buf->record->EndRecPtr, abort_time);
}
- ReorderBufferCleanupSubTxnTupleCids(ctx->reorder, xid, xid);
ReorderBufferAbort(ctx->reorder, xid, buf->record->EndRecPtr,
abort_time);
}
diff --git a/src/backend/replication/logical/reorderbuffer.c
b/src/backend/replication/logical/reorderbuffer.c
index e1c6a7c4d32..ccbdbde7335 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -240,6 +240,9 @@ static ReorderBufferTXN
*ReorderBufferTXNByXid(ReorderBuffer *rb,
XLogRecPtr lsn, bool create_as_top);
static void ReorderBufferTransferSnapToParent(ReorderBufferTXN *txn,
ReorderBufferTXN *subtxn);
+static bool TransactionIdInSubxactArray(TransactionId xid,
+
TransactionId *subxacts,
+
int nsubxacts);
static void AssertTXNLsnOrder(ReorderBuffer *rb);
@@ -3158,8 +3161,8 @@ ReorderBufferAbort(ReorderBuffer *rb, TransactionId xid,
XLogRecPtr lsn,
}
/*
- * Remove tuplecid changes queued by subtransaction xid from its toplevel
- * transaction's list.
+ * Remove tuplecid changes queued by aborted subtransactions from their
+ * toplevel transaction's list.
*
* Unlike regular changes, tuplecid changes are always queued on the toplevel
* transaction (see ReorderBufferAddNewTupleCids), so they would otherwise
@@ -3174,15 +3177,10 @@ ReorderBufferAbort(ReorderBuffer *rb, TransactionId
xid, XLogRecPtr lsn,
*
* The cleanup is pointless when the whole toplevel transaction is
* being aborted, since ReorderBufferCleanupTXN() frees the whole list
- * anyway; the caller passes the abort record's primary xid and we skip
- * the scan when it is xid's toplevel. Note that the opposite decision
- * -- cleaning only when the primary xid's own association is known --
- * would be wrong: abort records are written once the subtransaction is
- * already in TRANS_ABORT, so they carry no toplevel xid in their
- * header, and an outer subtransaction that never wrote WAL of its own
- * never gets an association at all. When such an outer subtransaction
- * rolls back, released inner subtransactions listed in its abort record
- * do have known associations and are cleaned here.
+ * anyway. If the primary xid is unknown, search its aborted children for a
known
+ * association. An outer subtransaction that never wrote WAL of its own can
+ * have no association, while released inner subtransactions listed in its
+ * abort record do have one and their tuplecids still need to be removed.
*
* If this pass does not know the association between the aborting
* subtransaction and its toplevel, there is nothing we can clean up here, and
@@ -3198,8 +3196,10 @@ ReorderBufferAbort(ReorderBuffer *rb, TransactionId xid,
XLogRecPtr lsn,
* transaction state without ever reaching ReorderBufferBuildTupleCidHash().
*/
void
-ReorderBufferCleanupSubTxnTupleCids(ReorderBuffer *rb, TransactionId xid,
-
TransactionId primary_xid)
+ReorderBufferCleanupAbortedSubTxnTupleCids(ReorderBuffer *rb,
+
TransactionId xid,
+
int nsubxacts,
+
TransactionId *subxacts)
{
ReorderBufferTXN *txn;
ReorderBufferTXN *toptxn;
@@ -3208,19 +3208,39 @@ ReorderBufferCleanupSubTxnTupleCids(ReorderBuffer *rb,
TransactionId xid,
txn = ReorderBufferTXNByXid(rb, xid, false, NULL, InvalidXLogRecPtr,
false);
- /* unknown transaction, or unknown association: nothing to remove */
- if (txn == NULL || !rbtxn_is_known_subxact(txn))
+ /*
+ * If xid is a known toplevel transaction, the whole transaction is
being
+ * aborted and ReorderBufferCleanupTXN() will free the tuplecid list.
+ */
+ if (txn != NULL && !rbtxn_is_known_subxact(txn))
return;
- toptxn = rbtxn_get_toptxn(txn);
-
/*
- * The whole toplevel transaction is being aborted (the abort record's
- * primary xid is xid's toplevel): ReorderBufferCleanupTXN() will free
- * the whole list shortly, don't scan it once per aborted subxid.
+ * If the aborting subtransaction is unknown, try to find the toplevel
+ * transaction through one of its children. There may be any number of
+ * unknown children, but they cannot have queued tuplecid changes:
decoding
+ * a WAL record that queues such a change first associates its xid with
the
+ * toplevel transaction. Therefore, if all children are unknown, there
is
+ * nothing to remove.
*/
- if (toptxn->xid == primary_xid)
- return;
+ if (txn == NULL)
+ {
+ for (int i = 0; i < nsubxacts; i++)
+ {
+ txn = ReorderBufferTXNByXid(rb, subxacts[i], false,
NULL,
+
InvalidXLogRecPtr, false);
+ if (txn != NULL && rbtxn_is_known_subxact(txn))
+ break;
+
+ txn = NULL;
+ }
+
+ if (txn == NULL)
+ return;
+ }
+
+ /* Get the top-level transaction to clean up its tuplecid list */
+ toptxn = rbtxn_get_toptxn(txn);
dlist_foreach_modify(it, &toptxn->tuplecids)
{
@@ -3230,15 +3250,38 @@ ReorderBufferCleanupSubTxnTupleCids(ReorderBuffer *rb,
TransactionId xid,
Assert(change->action ==
REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID);
- if (change->data.tuplecid.subxid == xid)
+ /*
+ * Remove the entry if it's part of the aborting subtransaction
or one
+ * of its children.
+ */
+ if (change->data.tuplecid.subxid == xid ||
+
TransactionIdInSubxactArray(change->data.tuplecid.subxid,
+
subxacts, nsubxacts))
{
dlist_delete(&change->node);
ReorderBufferFreeChange(rb, change, false);
+ Assert(toptxn->ntuplecids > 0);
toptxn->ntuplecids--;
}
}
}
+/*
+ * Check whether xid is in an array sorted in logical XID order.
+ */
+static bool
+TransactionIdInSubxactArray(TransactionId xid, TransactionId *subxacts,
+ int nsubxacts)
+{
+ /*
+ * An abort record's subxacts are children that previously subcommitted
+ * into the aborting transaction. AtSubCommit_childXids() preserves
their
+ * logical XID order, so xidLogicalComparator can safely compare them.
+ */
+ return bsearch(&xid, subxacts, nsubxacts,
+ sizeof(TransactionId), xidLogicalComparator)
!= NULL;
+}
+
/*
* Abort all transactions that aren't actually running anymore because the
* server restarted.
diff --git a/src/include/replication/reorderbuffer.h
b/src/include/replication/reorderbuffer.h
index 23c447c4953..7cfa43ad163 100644
--- a/src/include/replication/reorderbuffer.h
+++ b/src/include/replication/reorderbuffer.h
@@ -743,8 +743,10 @@ extern void ReorderBufferCommitChild(ReorderBuffer *rb,
TransactionId xid,
XLogRecPtr end_lsn);
extern void ReorderBufferAbort(ReorderBuffer *rb, TransactionId xid,
XLogRecPtr lsn,
TimestampTz
abort_time);
-extern void ReorderBufferCleanupSubTxnTupleCids(ReorderBuffer *rb,
TransactionId xid,
-
TransactionId primary_xid);
+extern void ReorderBufferCleanupAbortedSubTxnTupleCids(ReorderBuffer *rb,
+
TransactionId xid,
+
int nsubxacts,
+
TransactionId *subxacts);
extern void ReorderBufferAbortOld(ReorderBuffer *rb, TransactionId
oldestRunningXid);
extern void ReorderBufferForget(ReorderBuffer *rb, TransactionId xid,
XLogRecPtr lsn);
extern void ReorderBufferInvalidate(ReorderBuffer *rb, TransactionId xid,
XLogRecPtr lsn);
