Re: Bug in logical decoding with DDL and subtransactions

Hi,

On Mon, Oct 5, 2026 at 6:13 PM Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> 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.

I personally think your version looks better. But I have few comments for the
diff:

1.

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;

I'm curious whether the rbtxn_is_known_subxact() check is necessary here, it
gives me the impression that it's possible for an XID in the subxacts array to
be considered as a top-level transaction.

2.

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

I think this comment would be better placed at the caller.

--

And I also reviewed the corresponding part in the original patch, and have a
few comments:

3.

> * Note that we must not try to decide that from the primary xid's
> * own association instead: an abort record is written once the
> * subtransaction is already in TRANS_ABORT, so it never carries
> * the toplevel xid in its header (IsSubxactTopXidLogPending()
> * requires IsTransactionState()), and an outer subtransaction
> * that never wrote WAL of its own -- e.g. a savepoint that only
> * wraps other savepoints -- can therefore have no association at
> * all, while the released inner subtransactions it rolls back do
> * have one and their tuplecids still need to be removed.

As a reader, it wasn't initially clear to me what this comment is trying to
describe, since the association check is inside the cleanup function, so I
think it's better moved there.

It's also not clear to me what "try to decide that" refers to. I think it might
be saying that even if the primary XID's entry doesn't record the toplevel
transaction, we still need to do the cleanup by getting that info from another
subxact entry in the subxacts list. If so, we'd better make that explicit.

4.

* If this pass does not know the association between the aborting
* subtransaction and its toplevel, there is nothing we can clean up here, and
* that's fine: such a pass must have started after the subtransaction's first
* (toplevel-xid-bearing) WAL record. A pass that will output-decode the
* toplevel commit cannot have started that late: a slot's restart point
* cannot advance past the oldest in-progress transaction
* (SnapBuildProcessRunningXacts()), so the commit-decoding pass has replayed
* the record that established the association. Conversely, once the restart
* point has advanced past that record, the commit has already been consumed
* by an earlier pass and is skipped here via SnapBuildXactNeedsSkip(), so
* stale tuplecid entries left behind in that case are dropped along with the
* transaction state without ever reaching ReorderBufferBuildTupleCidHash().

It's not clear to me whether the comment is referring to a case that can
actually be hit. It sounds like it can happen and we just skip the cleanup, if
so, could you share an example so I can test it? If it's actually unreachable,
the comment shall explain it from that angle.

Best Regards,
Zhijie Hou