While we are on the topic of comments from lwlock.c, there is one other one
that confused me, in LWLockWaitListLock:
* /* and then spin without atomic operations until lock is released */ {
SpinDelayStatus delayStatus; init_local_spin_delay(&delayStatus); while
(old_state & LW_FLAG_LOCKED) { perform_spin_delay(&delayStatus); old_state
= pg_atomic_read_u32(&lock->state); }#ifdef LWLOCK_STATS delays +=
delayStatus.delays;#endif finish_spin_delay(&delayStatus); }*
It seems that we *are* using an atomic operation in the loop (though, no
compare-and-set, etc.) I might be mis-reading the intent of the comment,
but I'm curious if there's a way to reword it, too.
On Sat, Jan 25, 2025 at 4:06 PM Jacob Brazeal <jacob.braz...@gmail.com>
wrote:
> Thinking a bit further about this, the purpose of the LW_SHARED_MASK
> section of the state is to count the number of lock-sharers. Thus, we only
> care about the actual number of backends (up to 2^18-1) here and not the
> size of the ProcNumber data type. So I do think the comment should read
> 2^18-1 and not 2^23-1. Here is a patch to that effect.
>
> On Sat, Jan 25, 2025 at 3:21 PM Jacob Brazeal <jacob.braz...@gmail.com>
> wrote:
>
>> Hello all,
>>
>> In lwlocks.c, we have the following comment, related to LWLock state:
>>
>>
>> */* Must be greater than MAX_BACKENDS - which is 2^23-1, so we're fine.
>> */#define LW_SHARED_MASK ((uint32) ((1 << 24)-1))*
>>
>> However, MAX_BACKENDS is set to 2^18-1. Here is the comment in
>> postmaster.h:
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> */* * Note: MAX_BACKENDS is limited to 2^18-1 because that's the width
>> reserved * for buffer references in buf_internals.h. This limitation could
>> be lifted * by using a 64bit state; but it's unlikely to be worthwhile as
>> 2^18-1 * backends exceed currently realistic configurations. Even if that
>> limitation * were removed, we still could not a) exceed 2^23-1 because
>> inval.c stores * the ProcNumber as a 3-byte signed integer, b) INT_MAX/4
>> because some places * compute 4*MaxBackends without any overflow check.
>> This is rechecked in the * relevant GUC check hooks and in
>> RegisterBackgroundWorker(). */#define MAX_BACKENDS 0x3FFFF*
>>
>> 2^23-1 is noted as an additional upper limit, but I wonder if it'd be
>> correct to update the comment in lwlocks.c to
>>
>>
>> */* Must be greater than MAX_BACKENDS - which is 2^18-1, so we're fine.
>> */*
>>
>> I'm not sure if this could lead to us actually saving some bits in the
>> lwlock state, or if we could do anything useful with them anyway.
>>
>> Jacob
>>
>
