Re: lastOverflowedXid does not handle transaction ID wraparound

x4mmm@yandex-team.ru

From: Andrey Borodin <x4mmm@yandex-team.ru>
To: Alexander Korotkov <aekorotkov@gmail.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, Stan Hu <stanhu@gmail.com>, Dmitry Dolgov <9erthalion6@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2021-11-03T09:32:51Z
Lists: pgsql-hackers

> 3 нояб. 2021 г., в 14:08, Alexander Korotkov <aekorotkov@gmail.com> написал(а):
> 
> ( a.On Wed, Nov 3, 2021 at 11:44 AM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
>>> 21 окт. 2021 г., в 09:01, Kyotaro Horiguchi <horikyota.ntt@gmail.com> написал(а):
>>> 
>>> If the discussion so far is correct, the following diff will fix the
>>> issue.
>>> 
>>> diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
>>> index bd3c7a47fe..19682b73ec 100644
>>> --- a/src/backend/storage/ipc/procarray.c
>>> +++ b/src/backend/storage/ipc/procarray.c
>>> @@ -4463,6 +4463,12 @@ ExpireOldKnownAssignedTransactionIds(TransactionId xid)
>>> {
>>>       LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
>>>       KnownAssignedXidsRemovePreceding(xid);
>>> +       /*
>>> +        * reset lastOverflowedXid if we know transactions that have been possiblly
>>> +        * running are being gone.
>>> +        */
>>> +       if (TransactionIdPrecedes(procArray->lastOverflowedXid, xid))
>>> +               procArray->lastOverflowedXid = InvalidTransactionId;
>>>       LWLockRelease(ProcArrayLock);
>>> }
>> 
>> The patch seems correct bugfix to me. The only question I have: is it right place from modularity standpoint? procArray->lastOverflowedXid is not a part of KnownAssignedTransactionIds?
> 
> It seems the right place because we take ProcArrayLock here.
Oh.. I see. ProcArrayApplyRecoveryInfo() is taking ProcArrayLock in so many places.

>  It would
> be undesirable to take it twice.  We could give a better name for
> ExpireOldKnownAssignedTransactionIds() indicating that it could modify
> lastOverflowedXid as well.  Any ideas?
Looking more I think the name is OK. KnownAssignedXidsReset() and KnownAssignedXidsRemovePreceding() interferes with procArray a lot.

> Should ExpireAllKnownAssignedTransactionIds() be also involved here?
I think it's good for unification, but I do not see how procArray->lastOverflowedXid can be used after ExpireAllKnownAssignedTransactionIds().


Best regards, Andrey Borodin.


Commits

  1. Reset lastOverflowedXid on standby when needed

  2. Fix variable shadowing in procarray.c.

  3. Move InRecovery and standbyState global vars to xlogutils.c.