Re: Bug in logical decoding with DDL and subtransactions

Hi Álvaro,

Thanks for picking this up.

> What's the reason to consider this as a worse problem in 19 than in
> previous releases?

There isn't one -- I asked because 19 GA was approaching and the bug
was reported against 19. Fixing it in all stable branches for the
November minors sounds right.

> If the proposed patch has any conflicts when applied on any previous
> branches, then by all means please do provide backpatches as
> appropriate.

I ran v4 across all supported branches (--enable-cassert build plus
the full test_decoding suite on each):

branch apply tests (regress+isolation)
------ ------------------------------- -------------------------
master clean (be00f041a33) 21+15, CFBot green
19 clean (a203d201900) 21+15
18 trivial test-list conflicts 20+14
17 branch version attached 20+14
16 branch version attached 20+14
15 branch version attached 20+14
14 branch version attached 20+13

(The counts follow each branch's existing test_decoding suite;
tuplecid and tuplecid_restart pass everywhere.)

On 18 the conflicts are exactly the trivial ones you saw: master's
test lists mention repack and parallel_session_origin, which don't
exist there. All code applies cleanly, so per your advice I have not
prepared an 18 version.

The attached branch versions differ from v4 as follows:

- 17, 16: only the test-list context, plus one rename -- the cleanup
calls ReorderBufferReturnChange(), the pre-PG18 name of
ReorderBufferFreeChange() (renamed by ac4494646da; same 3-argument
signature and body). The 17 and 16 versions differ from each other
only in the test-list context, too.
- 15, 14: additionally drop the meson.build hunk (meson arrived in
16) and use the pre-PG16 names: RelFileNode, xlrec->target_node,
and txn->toptxn instead of rbtxn_get_toptxn(). The 14 version also
trims the new test's expected output, because 14's psql prints only
the last result of a multi-statement string; the decode work is
unchanged, so the test exercises the bug just the same.

rbtxn_is_known_subxact() exists natively in all of these branches, so
the fix itself is identical everywhere.

> The patch does change the layout of ReorderBufferChange, which is
> apparently an exported struct, so some analysis on whether that can
> break anything would also be useful.

I checked who can observe the layout change:

- the tuplecid variant is only ever accessed from reorderbuffer.c
itself (no data.tuplecid users anywhere else in tree);
- only reorderbuffer.c touches change->node, and only reorderbuffer.c
allocates, frees or serializes ReorderBufferChange (the spill files
embed the struct, but they are transient within a single decoding
session);
- output plugins (pgoutput, pgrepack, test_decoding, and external
ones using the same callback API) only read change->action, ->txn,
->data.tp.{old,new}tuple and ->data.truncate.*, all at offsets
unchanged by the patch: the union only grows at the tail of the
tuplecid variant, and the struct stays within two cachelines, as
your pahole output shows.

So the layout change is innocuous for any plugin I can think of.

There is one more exported-API change next to it:
ReorderBufferAddNewTupleCids() gains a subxid parameter. Its only
caller is the NEW_CID decode path in snapbuild.c; output plugins have
no reason to call it. If you'd rather keep the exported signature
stable on the back branches, I can keep the old one as a wrapper
passing InvalidTransactionId -- it is about five lines.

The part of v4 where I would still most value a second opinion is the
restart_lsn invariant in section 2 of the v4 mail (why the skipped
cleanup on the restart path cannot reach the tuplecid hash); I believe
everything else is covered above.

Thanks,
Bingshuai Li