RE: Fix premature xmin advancement during fast forward decoding

On Fri, Apr 25, 2025 at 5:44 AM Masahiko Sawada wrote:

(Resending this email after compressing the attachment)

> On Tue, Apr 22, 2025 at 12:06 AM Zhijie Hou (Fujitsu)
> <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> >
> > Hi,
> >
> > When analyzing some issues in another thread[1], I found a bug that
> > fast forward decoding could lead to premature advancement of
> > catalog_xmin, resulting in required catalog data being removed by vacuum.
> >
> > The issue arises because we do not build a base snapshot when decoding
> > changes during fast forward mode, preventing reference to the minimum
> > transaction ID that remains visible in the snapshot when determining
> > the candidate for catalog_xmin. As a result, catalog_xmin was directly
> > advanced to the oldest running transaction ID found in the latest
> running_xacts record.
> >
> > In code-level, SnapBuildProcessChange -> ReorderBufferSetBaseSnapshot
> > could not be reached during fast forward decoding, resulting in
> > rb->txns_by_base_snapshot_lsn being NULL. When advancing
> catalog_xmin,
> > rb->the
> > system attempts to refer to rb->txns_by_base_snapshot_lsn via
> > ReorderBufferGetOldestXmin(). However, since
> > rb->txns_by_base_snapshot_lsn is NULL, it defaults to directly using
> > running->oldestRunningXid as the candidate for catalog_xmin. For
> > reference, see the details in SnapBuildProcessRunningXacts.
>
> I agree with your analysis.
>
> > To fix this, I think we can allow the base snapshot to be built during
> > fast forward decoding, as implemented in the patch 0001 (We already
> > built base snapshot in fast-forward mode for logical message in
> logicalmsg_decode()).
> >
> > I also explored the possibility of further optimizing the fix to
> > reduce the overhead associated with building a snapshot in
> > fast-forward mode. E.g., to maintain a single base_snapshot_xmin
> > rather than generating a full snapshot for each transaction, and use
> > this base_snapshot_xmin when determining the candidate catalog_xmin.
> > However, I am not feeling confident about maintaining some
> > fast-forward dedicated fields in common structures and perhaps employing
> different handling for catalog_xmin.
> >
> > Moreover, I conducted a basic test[2] to test the patch's impact,
> > noting that advancing the slot incurs roughly a 4% increase in
> > processing time after applying the patch, which appears to be
> > acceptable. Additionally, the cost associated with building the
> > snapshot via SnapBuildBuildSnapshot() did not show up in the profile.
>
> Where did the regression come from? I think that even with your patch
> SnapBuildBuildSnapshot() would be called only a few times in the workload.

AFAICS, the primary cost arises from the additional function
invocations/executions. Functions such as SnapBuildProcessChange,
ReorderBufferSetBaseSnapshot, and ReorderBufferXidHasBaseSnapshot are invoked
more frequently after the patch. Although these functions aren't inherently
expensive, the scenario I tested involved numerous transactions starting with
just a single insert each. This resulted in a high frequency of calls to these
functions.

Attaching the profile for both HEAD and PATCHED for reference.

> With the patch, I think that we need to create ReorderBufferTXN entries for
> each transaction even during fast_forward mode, which could lead to
> overheads.

I think ReorderBufferTXN was created in fast_forward even without the patch.
See heap_decode-> ReorderBufferProcessXid.

> I think that 4% degradation is something that we want to avoid.

As mentioned above, these costs arise from essential function executions
required to maintain txns_by_base_snapshot_lsn. So I think the cost is
reasonable to ensure correctness. Thoughts ?

Best Regards,
Hou zj