Thread: Lock on ShmemVariableCache fields?

Hi, hackers

The VariableCacheData says nextOid and oidCount are protected by
OidGenLock.  However, we update them without holding the lock on
OidGenLock in BootStrapXLOG().  Same as nextXid, for other fields
that are protected by XidGenLock, it holds the lock, see
SetTransactionIdLimit().

void
BootStrapXLOG(void)
{
    [...]

    ShmemVariableCache->nextXid = checkPoint.nextXid;
    ShmemVariableCache->nextOid = checkPoint.nextOid;
    ShmemVariableCache->oidCount = 0;

    [...]

    SetTransactionIdLimit(checkPoint.oldestXid, checkPoint.oldestXidDB);

    [...]
}

I also find a similar code in StartupXLOG().  Why we don't hold the lock
on OidGenLock when updating ShmemVariableCache->nextOid and
ShmemVariableCache->oidCount?

If the lock is unnecessary, I think adding some comments is better.

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Mon, 31 Oct 2022 at 14:15, Zhang Mingli <zmlpostgres@gmail.com> wrote:
> HI,
>
> On Oct 31, 2022, 10:48 +0800, Japin Li <japinli@hotmail.com>, wrote:
>>
>> I also find a similar code in StartupXLOG(). Why we don't hold the lock
>> on OidGenLock when updating ShmemVariableCache->nextOid and
>> ShmemVariableCache->oidCount?
>>
>> If the lock is unnecessary, I think adding some comments is better.
>>
> As its name BootStrapXLOG, it’s used in BootStrap mode to initialize the template database.
> The process doesn’t speak SQL and the database is not ready.
> There won’t be  concurrent access to variables.
>

Thanks for your explanation.  I got your mind.  So, in theory, we can also update
everything in ShmemVariableCache without a lock?

For example, since SetCommitTsLimit() is only used in BootStrapXLog() and
StartupXLOG(), we can safely remove the code of acquiring/releasing lock?

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

On Mon, Oct 31, 2022 at 03:14:54PM +0800, Japin Li wrote:
> For example, since SetCommitTsLimit() is only used in BootStrapXLog() and
> StartupXLOG(), we can safely remove the code of acquiring/releasing lock?

Logically yes, I guess that you could go without the LWLock acquired
in this routine at this early stage of the game.  Now, perhaps that's
not worth worrying, but removing these locks could impact any external
code relying on SetCommitTsLimit() to actually hold them.
--
Michael

On 2022-Nov-01, Michael Paquier wrote:

> On Mon, Oct 31, 2022 at 03:14:54PM +0800, Japin Li wrote:
> > For example, since SetCommitTsLimit() is only used in BootStrapXLog() and
> > StartupXLOG(), we can safely remove the code of acquiring/releasing lock?
> 
> Logically yes, I guess that you could go without the LWLock acquired
> in this routine at this early stage of the game.  Now, perhaps that's
> not worth worrying, but removing these locks could impact any external
> code relying on SetCommitTsLimit() to actually hold them.

My 0.02€:  From an API point of view it makes no sense to remove
acquisition of the lock in SetCommitTsLimit, particularly given that
that function is not at all performance critical.  I think the first
question I would ask, when somebody proposes to make a change like that,
is *why* do they want to make that change.  Is it just because we *can*?
That doesn't sound, to me, sufficient motivation.  You actually
introduce *more* complexity, because after such a change any future
hacker would have to worry about whether their changes are still valid
considering that these struct members are modified unlocked someplace.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
Syntax error: function hell() needs an argument.
Please choose what hell you want to involve.