Re: Bug in logical decoding with DDL and subtransactions

Hi,

On Thu, Oct 8, 2026 at 5:16 PM Bingshuai Li <lucian1412(at)outlook(dot)com> wrote:
>
> On Zhijie's comments:
>
> 1. I think the check is needed on a pass that restarts after a child's
> first WAL record. Later records can create a ReorderBuffer entry without
> replaying the record that established its toplevel association. The
> existing catalog_change_snapshot test describes this for NEW_CID records.
> I reproduced it with a nested variant on current master plus v6 and the
> guard above: at the outer abort, the primary XID was unknown, while a
> child in the subxacts array had an entry but was not a known subxact.
> The toplevel commit was skipped in that pass. So an entry's presence
> does not establish its association; I'd keep rbtxn_is_known_subxact()
> and explain that reason in the comment.
>
> 2. Agreed, the sortedness comment belongs at the call site.
>
> 3. Agreed. 0002 already rewrote the helper's comment. The decode.c
> comment could just explain why cleanup precedes the ReorderBufferAbort()
> loop, leaving the association reasoning in the helper.
>
> 4. Yes, the skipped-cleanup case is reachable on a pass that restarts
> after the subtransaction's first, toplevel-XID-bearing WAL record.
> Later NEW_CID records can still queue tuplecids on the toplevel without
> establishing the writer's association. I reproduced this by consuming
> the toplevel commit and then replaying from a checkpoint between the
> child's first WAL record and its catalog write. The abort cleanup was
> skipped, and so was that already-consumed commit.
>
> For a small example using catalog_change_snapshot.spec, add this step
> to session s0 and run only the following permutation:
>
> step "s0_rollback" { ROLLBACK TO SAVEPOINT sp1; }
>
> permutation "s0_init" "s0_begin" "s0_savepoint" "s0_insert"
> "s1_checkpoint" "s1_get_changes" "s0_insert2"
> "s0_rollback" "s0_commit" "s0_begin" "s0_insert"
> "s1_checkpoint" "s1_get_changes" "s0_commit"
> "s1_get_changes"
>
> The final get_changes replays the abort without the association and
> skips the earlier commit. To exercise (1), nest another savepoint inside
> sp1 before the first insert, and release it just before s0_rollback.

Thanks for the explanation. I re-read the code and comments and have some
suggestions to simplify the code and improve some comments. I think we can
reduce the code in Kuroda-san's patch with a simpler loop:

txn = ReorderBufferTXNByXid(rb, xid, false, NULL, InvalidXLogRecPtr,
false);

for (int i = 0; txn == NULL && i < nsubxacts; i++)
txn = ReorderBufferTXNByXid(rb, subxacts[i], false, NULL,
InvalidXLogRecPtr, false);

followed by a single early return covering all the skip cases:

if (txn == NULL || !rbtxn_is_known_subxact(txn))
return;

I think this makes the code cleaner and shorter, and also fixes the BF failure
IIUC.

I also rewrote some of the comments above
ReorderBufferCleanupAbortedSubTxnTupleCids(), using wording that's more
consistent with the rest of the codebase and simplifying the explanation of
logic that isn't directly related, hoping it reads better.

I discussed this with Kuroda-san off-list, and have merged the suggested
changes into the original 0002 diff shared by Kuroda-san.

I also moved the tests that Alvaro suggested to remove out of 0001, keeping
them as 0003, so that CFbot can still test them for now, and we can commit
without them later.

0001 is unchanged.

Best Regards,
Zhijie Hou