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)
