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)
