Re: TransactionXmin != MyProc->xmin

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)