Horton

Thread

Commits

  1. Update TransactionXmin when MyProc->xmin is updated

  1. TransactionXmin != MyProc->xmin

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-12-12T18:16:39Z

    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)

  2. Re: TransactionXmin != MyProc->xmin

    Andres Freund <andres@anarazel.de> — 2024-12-12T19:57:57Z

    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

  3. Re: TransactionXmin != MyProc->xmin

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-12-12T20:26:39Z

    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)

  4. Re: TransactionXmin != MyProc->xmin

    Heikki Linnakangas <hlinnaka@iki.fi> — 2024-12-21T22:12:55Z

    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)