Hi, On 2024-12-12 20:16:39 +0200, Heikki Linnakangas wrote: > The comment in GetSnapshotData() defines transactionXmin like this: > > > TransactionXmin: the oldest xmin of any snapshot in use in the current > > transaction (this is the same as MyProc->xmin). > However, we don't update TransactionXmin when we update MyProc->xmin in > SnapshotResetXmin(). So TransactionXmin can be older than MyProc->xmin.
Oops, good catch. > A straightforward fix is to ensure that TransactionXmin is updated whenever > MyProc->xmin is: > > diff --git a/src/backend/utils/time/snapmgr.c > b/src/backend/utils/time/snapmgr.c > index a1a0c2adeb6..f59830abd21 100644 > --- a/src/backend/utils/time/snapmgr.c > +++ b/src/backend/utils/time/snapmgr.c > @@ -883,7 +883,7 @@ SnapshotResetXmin(void) > > pairingheap_first(&RegisteredSnapshots)); > > if (TransactionIdPrecedes(MyProc->xmin, minSnapshot->xmin)) > - MyProc->xmin = minSnapshot->xmin; > + MyProc->xmin = TransactionXmin = minSnapshot->xmin; > } > > /* > > Anyone see a reason not to do that? Seems to make sense. But shouldn't we reset TransactionXmin in the pairingheap_is_empty() case as well? Perhaps we should have some assertions ensuring TransactionXmin has a valid value in some places? > There are a two other places where we set MyProc->xmin without updating > TransactionXmin: > > 1. In ProcessStandbyHSFeedbackMessage(). AFAICS that's OK because walsender > doesn't use TransactionXmin for anything. It's worth noting that a walsender connection can do normal query processing these days if connected to a database.... > 2. In SnapBuildInitialSnapshot(). I suppose that's also OK because the > TransactionXmin isn't used. I don't quite remember when that function might > be called though. It's used to export a snapshot after creating a logical slot. The snapshot can be used to sync the existing data, before starting to apply future changes. I don't see a need to modify TransactionXmin here, it's not a normal snapshot, and as a comment in the function says, we rely on the slot's xmin to protect against relevant rows being removed. > In any case, I propose that we set TransactionXmin in all of those cases as > well, so that TransactionXmin is always the equal to MyProc->xmin. Maybe > even rename it to MyProcXmin to make that more clear. I'm not sure it's really right to do that. If we don't hold a snapshot, what makes sure that we then call SnapshotResetXmin() or such to reset TransactionXmin? Greetings, Andres Freund
