Thread: Re: TransactionXmin != MyProc->xmin
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
On 12/12/2024 21:57, Andres Freund wrote: > On 2024-12-12 20:16:39 +0200, Heikki Linnakangas wrote: >> 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? Yes, good catch. > Perhaps we should have some assertions ensuring TransactionXmin has a valid > value in some places? +1, wouldn't hurt. >> 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? I don't understand. What I was meant is that we make it a strict rule that whenever you change MyProc->xmin, you always update TransactionXmin too, so that TransactionXmin == MyProc->xmin is always true. -- Heikki Linnakangas Neon (https://neon.tech)
On 12/12/2024 22:26, Heikki Linnakangas wrote: > On 12/12/2024 21:57, Andres Freund wrote: >> Perhaps we should have some assertions ensuring TransactionXmin has a >> valid >> value in some places? > > +1, wouldn't hurt. I didn't, after all, as I couldn't find a good place where to put that. >>> 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.... Hmm; they will use regular snapshots in that case, which will set TransactionXmin. But yes, good point nevertheless. >>> 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? > > I don't understand. What I was meant is that we make it a strict rule > that whenever you change MyProc->xmin, you always update TransactionXmin > too, so that TransactionXmin == MyProc->xmin is always true. I committed a minimal fix in SnapshotResetXmin() only, in all stable branches. I still think it might be a good idea to make it a strict invariant that TransactionXmin == MyProc->xmin is always true. But it's not wrong as it is, if you think that TransactionXmin is the xmin of the oldest active snapshot in the system, so that it might not be set if MyProc->xmin is held back by something else than a snapshot, like in the walsender processes. -- Heikki Linnakangas Neon (https://neon.tech)