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