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.f...@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. 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
<<attachment: profiles.zip>>
