Re: Bug in logical decoding with DDL and subtransactions

Hi,

I noticed that with beff3cd0098, we scan the whole tuplecid list even
when the aborted subtransaction only did DML. If there's DDL earlier in
the same transaction, we keep walking the list without removing anything.

Could we skip that scan when none of the aborting xids has catalog changes?
Something like this:

diff --git a/src/backend/replication/logical/reorderbuffer.c
b/src/backend/replication/logical/reorderbuffer.c
index 6c4eadc9972..57a72553d03 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -3297,6 +3297,23 @@ ReorderBufferCleanupAbortedTupleCids(ReorderBuffer *rb,
if (txn == NULL || !rbtxn_is_known_subxact(txn))
return;

+ /*
+ * SnapBuildProcessNewCid() marks each xid that queues tuplecids as having
+ * catalog changes. Skip the scan unless an aborting xid has that flag.
+ * Check subcommitted children too, since their parent may not have made
+ * catalog changes itself.
+ */
+ if (!rbtxn_has_catalog_changes(txn))
+ {
+ int subidx;
+
+ for (subidx = 0; subidx < nsubxacts; subidx++)
+ if (ReorderBufferXidHasCatalogChanges(rb, subxacts[subidx]))
+ break;
+ if (subidx == nsubxacts)
+ return;
+ }
+
/* Get the top-level transaction whose tuplecid list we're to clean */
toptxn = rbtxn_get_toptxn(txn);

Without the loop over the children, the cmin assertion fails if I add an
insert at the outer savepoint in tuplecid_nested.

This should save some work when a transaction has built up a long tuplecid
list and then hits lots of DML-only subtransaction aborts. I can prepare a
patch if the direction looks right.

Regards,
Ayush