Re: Historic snapshot doesn't track txns committed inBUILDING_SNAPSHOT state

Hi,

On Sat, Oct 17, 2026 at 12:40 AM Álvaro Herrera <alvherre(at)kurilemu(dot)de> wrote:
>
> On 2025-Nov-21, cca5507 wrote:
>
> > Attach the rebased patches.
>
> I rebased this again, for CFbot. I haven't reviewed it.
>
> > Discussion:
> > https://www.postgresql.org/message-id/flat/tencent_6AAF072A7623A11A85C0B5FD290232467808(at)qq(dot)com
> > https://www.postgresql.org/message-id/flat/tencent_8DEC9842690A9B6AFD52D4659EF0700E9409(at)qq(dot)com
> > https://www.postgresql.org/message-id/flat/tencent_FA60D4EE3E14ACF0B936396551260A4FFD05(at)qq(dot)com
> >
> > Bug Report:
> > https://www.postgresql.org/message-id/flat/18509-983f064d174ea880%40postgresql.org
> > https://www.postgresql.org/message-id/flat/19109-4a9ce36715d69632%40postgresql.org
> >
> > CF Entry:
> > https://commitfest.postgresql.org/patch/5029/

Thanks for updating the patches.

I reviewed the patches recently and have a few comments.

1.

+ * Note that we only need to do this when we are not fast-forwarding
+ * and there is a snapshot.
*/
if (TransactionIdIsValid(xid))
{
- if (!ctx->fast_forward)
+ if (!ctx->fast_forward && has_snapshot)
ReorderBufferAddInvalidations(reorder, xid,

The patch skips accumulating the invalidations, but that doesn't seem correct,
since we may now need to distribute the invalidations to concurrent transactions
(see ReorderBufferAddDistributedInvalidations()). So I think we should not add
this check.

2.

If we address point 1, then calling ReorderBufferForget() for transactions
collected during the BUILD state would trigger an Assert in
ReorderBufferForget(), as we don't have a snapshot for them yet. I think we
could simply remove this Assert and add some comments to explain this case.

if (txn->base_snapshot != NULL && txn->ninvalidations > 0)
ReorderBufferImmediateInvalidation(rb, txn->ninvalidations,
txn->invalidations);
else
Assert(txn->ninvalidations == 0);

3.

Since we now handle transactions during the BUILDING state, the following code
becomes reachable:

(builder->state == SNAPBUILD_BUILDING_SNAPSHOT &&
TransactionIdPrecedes(xid, builder->next_phase_at)))
{
/* ensure that only commits after this are getting replayed */
if (builder->start_decoding_at <= lsn)
builder->start_decoding_at = lsn + 1;
return;
}

But I realized it fails to check the subtransaction IDs. If the main transaction
ID precedes next_phase_at while a subtransaction ID is greater than it, I think
we'd also need to collect it into the snapshot in that case.

4.

@@ -436,7 +446,19 @@ heap2_decode(LogicalDecodingContext *ctx,
XLogRecordBuffer *buf)
* SnapBuildProcessRunningXacts().
*/
if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT)
+ {
+ /*
+ * If we are building snapshot and the xlog means a catalog
+ * change, we need to mark it in the reorder buffer.
+ *
+ * Now only XLOG_HEAP2_NEW_CID means a catalog change.
+ */
+ if (SnapBuildCurrentState(builder) >= SNAPBUILD_BUILDING_SNAPSHOT &&
+ TransactionIdIsValid(xid) && info == XLOG_HEAP2_NEW_CID)
+ ReorderBufferXidSetCatalogChanges(ctx->reorder, xid, buf->origptr);
+

I feel adding this check inside the switch-case branch looks better
than duplicating some if conditions in a new branch. Also, the above code will
mark the transaction as a catalog change even in fast_forward mode, while the
code in the latter switch branch does not. I believe marking it as a catalog
change in fast_forward mode is correct, because the snapshot built in that mode
could also be shared with other decoding sessions.

I've made a simple follow-up patch that addresses the above, see 0001.

BTW, the current test in the patch doesn't reproduce the issue: it passes even
without the fix. So I tried writing some tests covering the original report and
the corner cases I mentioned above, see 0002 (atop of v7-0002).

Best Regards,
Zhijie Hou