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

Hi,

Really sorry for the late reply, I only saw your mail now.

Thanks for the review. v3 is attached. I dropped the long comment and
changed the existing one as you suggested:

/* update the statistics and mark the transaction serialized iff we
spilled */

The code is otherwise unchanged from v2. The flag is still set at the end
of the "if (spilled)" block, after the spillTxns line that checks it.

With v3, all test_decoding regression (20) and isolation (16) tests pass on
master. The 17 and 14 backports are updated the same way, in case they help.
I did not re-run their tests after the comment change, since only the
comment differs from v2.

Regards,
Atish Prakash

On Mon, 9 Nov 2026 at 20:17, Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com>
wrote:

> Hi,
>
> On Mon, Nov 09, 2026 at 07:46:59PM +0800, Zhijie Hou wrote:
> > 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.
>
> LGTM, just one nit about:
>
> + /*
> + * Mark the transaction as serialized only if something
> was written,
> + * and only after the check above. final_lsn is advanced
> for each
> + * change written, so a transaction that wrote nothing,
> such as a
> + * subtransaction without decoded changes, would be marked
> with an
> + * invalid final_lsn, and ReorderBufferRestoreCleanup()
> would later
> + * fail on it.
> + */
>
>
> It looks lengthy and “the check above” is ambiguous. I'm not sure we need
> a new
> comment here, maybe just replace the existing?
>
> /* update the statistics iff we have spilled anything */
>
> with
>
> /* update the statistics and mark the transaction serialized iff we
> spilled */
>
>
> Regards,
>
> --
> Bertrand Drouvot
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com
>