Re: Bug in logical decoding with DDL and subtransactions

On 2026-Oct-09, Álvaro Herrera wrote:

> On 2026-10-09, Zhijie Hou wrote:
>
> > 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.
>
> Hmm I had already made essentially the same changes locally plus a few
> others. I'll share my patches over the weekend.

Thanks. It turned out that the changes I had done were not the same at
all, so I ended up the coding of the new routine essentially the same as
what's in your patch. But I didn't have time to fully merge my comment
updates with yours, so I kept mine in the function header comment and
mostly yours (but also partly mine) within the function. You can see I
added an #ifdef block around around the part you found objectionable
(which I also do). I will give these another read tomorrow to merge
everything in a sane manner.

The new comment in reorderbuffer.h was rewritten. I think I would like
to add an XXX at that spot, to point out that this is a somewhat silly
arrangement, and that we should instead make tuplecid changes attached
to the subtransaction rather than the top-level transaction, to make the
work at abort time more reasonble. (That would be a master-only patch,
of course.) I think XXX comments are effective to motivate people to do
that kind of work.

I renamed the new function to ReorderBufferCleanupAbortedTupleCids, and
moved the new subxid argument to another position.

This patch also renames the tuplecid_data global in snapmgr.c to
HistoricTupleCidHash. I intend to make this change in master only. I
think the other name is confusing considering that reorderbuffer.c has a
function parameter using the same name.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Los dioses no protegen a los insensatos. Éstos reciben protección de
otros insensatos mejor dotados" (Luis Wu, Mundo Anillo)