RE: Bug in logical decoding with DDL and subtransactions

Hi Bingshuai,

> Thanks Álvaro for putting together v6. In cfbot run 37698593816, the two
> UBSan jobs, "Linux - Meson (32-bit)" and "Linux - Autoconf", fail with
> the same error:

Embarrassingly, I had not found the cfbot entry. Good catch.
Let me confirm one point, why the test could pass for some platforms?

> The guard belongs in TransactionIdInSubxactArray():
>
> if (nsubxacts == 0)
> return false;

LGTM.

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

Even though the sub-transactions are not assigned as a child, the code works well
without the rbtxn_is_known_subxact(), right? If so we may not have to keep the
check.

Also, while thinking on it, I started to think whether we have to clean up
Tuplecids even if the transactions will be skipped. In this case transactions are
Not replied thus the issue won't happen, right?

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

I think the code comment atop the cleanup can be simper like:
```
/*
* Remove tuplecid changes queued by the aborted subtransactions
* from the toplevel transaction's list.
*/
ReorderBufferCleanupAbortedSubTxnTupleCids(ctx->reorder, xid,
```

Best regards,
Hayato Kuroda
FUJITSU LIMITED