Re: Fix premature xmin advancement during fast forward decoding - Mailing list pgsql-hackers

On Fri, Apr 25, 2025 at 8:14 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Fri, Apr 25, 2025 at 5:44 AM Masahiko Sawada wrote:
> > On Tue, Apr 22, 2025 at 12:06 AM Zhijie Hou (Fujitsu)
> > <houzj.fnst@fujitsu.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.
>

Yeah, I see below from the patched profile.

|--4.65%--SnapBuildProcessChange
                                                |          |
|          |
                                                |          |
|          |--2.08%--ReorderBufferSetBaseSnapshot
                                                |          |
|          |          |
                                                |          |
|          |           --1.32%--__mcount_internal
                                                |          |
|          |
                                                |          |
|          |--1.29%--__mcount_internal
                                                |          |
|          |
                                                |          |
|           --0.72%--ReorderBufferXidHasBaseSnapshot

As the number of operations per transaction increases, this cost
should reduce further as we will set base_snapshot only once per
transaction. Now, we can try to micro-optimize this by passing
ReorderBufferTXN, as that is computed before we invoke
SnapBuildProcessChange, but not sure how much it will change. However,
the bigger question is, is it really worth optimizing this code path?

If we really want to optimize this code path for the test Hou-San did,
I see that SnapBuildCommitTxn() has a bigger overhead, so we should
investigate whether we really need it for the fast-forward mode, where
we won't process changes. I am of the opinion that we need to pay this
additional cost of setting base_snapshot for the correctness of the
fast-forward mode.

--
With Regards,
Amit Kapila.