TransactionXmin != MyProc->xmin
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.
I browsed around to see if that might cause trouble, and found one such
case: SubTransGetTopmostTransaction() uses TransactionXmin as the
cut-off point so that it doesn't try to perform lookups of old XIDs that
might've already been truncated away from pg_subtrans. When
TransactionXmin is older than MyProc->xmin, then it might do that. The
attached isolation test demonstrates that case and produces an error:
ERROR: could not access status of transaction 17190290
DETAIL: Could not open file "pg_subtrans/0106": No such file or directory.
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?
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.
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.
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.
--
Heikki Linnakangas
Neon (https://neon.tech)
Attachments:
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)