Re: BUG #19839: Assert in ReorderBufferRestoreCleanup for aborted, spilled transaction

Hi,

On Mon, Nov 9, 2026 at 06:53 PM Bertrand Drouvot wrote:
>
> On Sun, Nov 08, 2026 at 11:11:09PM +0800, Zhijie Hou wrote:
> > Thanks for the report and patch.
>
> +1.
>
> > See the attachment for a test to reproduce this issue. (Feel free to merge
> > if it's worth adding a test to cover this path)
>
> > I think in a rare case, this can also affect the stats collection
> > for spilled txns. For example, if an empty subtransaction is marked
> > as
> > serialized, we skip incrementing spillTxns because no changes were written;
> > if real changes are later serialized in this subtransaction, we still do not
> > increment spillTxns because the subtransaction is already marked as
> > serialized and we avoid counting it "twice".
>
> Agreed.
>
> That's why I think the test is worth adding (as the bug could also
> produce real user visible impact as it can cause spill_txns to be
> undercounted on non assert enabled builds). The test only detects the
> regression on assert enabled builds, but I think that is enough. A
> test for the spill_txns case would be more complicated and is probably
> not worth it.
>
> >
> > + if (spilled)
> > + txn->txn_flags |= RBTXN_IS_SERIALIZED;
> >
> > ReorderBufferSerializeTXN() already has an "if (spilled)" block
> > right above this, so we shall directly move the assignment into that
> > block.
>
> +1.

Thanks for the review. It looks like Atisss's update patches didn't
reach the list, probably a missing Reply-All, so I'm helping to attach
the same patches (provided by Atisss) here.

--
Best Regards,
Zhijie Hou