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
