> On Jan 15, 2026, at 00:30, Andres Freund <[email protected]> wrote:
>
> Hi,
>
> On 2026-01-14 11:41:19 +0800, Chao Li wrote:
>> Basically, code changes in 0003 is straightforward, just a couple of small
>> comments:
>>
>> 1
>> ```
>> - * refcounts 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. We check that the configured
>> - * number of backends does not exceed MAX_BACKENDS in
>> InitializeMaxBackends().
>> + * refcounts in buf_internals.h. This limitation could be lifted, but it's
>> ```
>>
>> Before this patch, there was room for lifting the limitation. With this
>> patch, state is 64bit already, but the significant 32bit will be used for
>> buffer locking as stated in buf_internals.h, in other words, there is no
>> room for lifting the limitation now. If that’s true, then I think we can
>> remove the statements about lifting limitation.
>
> I'm not following - there's plenty space for more bits if we need that:
>
> * State of the buffer itself (in order):
> * - 18 bits refcount
> * - 4 bits usage count
> * - 12 bits of flags
> * - 18 bits share-lock count
> * - 1 bit share-exclusive locked
> * - 1 bit exclusive locked
>
> That's 54 bits in total. Which part is in the lower and which in the upper
> 32bit isn't relevant for anything afaict?
Because I saw the comment in buf_internals.h:
```
* NB: A future commit will use a significant portion of the remaining bits to
* implement buffer locking as part of the state variable.
```
That seems to indicate all the significant 32 bits will be used for buffer
locking. Also, there is an assert that concretes the impression:
```
StaticAssertDecl(BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS + BUF_FLAG_BITS == 32,
"parts of buffer state space need to equal 32");
```
So, I thought we can explain 18bit refcount is good enough without mentioning
“lifting” that potentially adds confusion to readers. But anyway, this is not a
strong opinion. I won’t insist on this comment.
>
>
>> 2. By searching for “LockBufHdr”, I found one place missed to update in
>> contrib/pg_prewarm/autoprewarm.c at line 706:
>> ```
>> for (num_blocks = 0, i = 0; i < NBuffers; i++)
>> {
>> uint32 buf_state; <=== line 706, should change to uint64
>>
>> CHECK_FOR_INTERRUPTS();
>>
>> bufHdr = GetBufferDescriptor(i);
>>
>> /* Lock each buffer header before inspecting. */
>> buf_state = LockBufHdr(bufHdr);
>> ```
>
> Good catch! I didn't find any other similar omissions...
I saw you have added this occurrence to v11.
>
>
>> I will continue reviewing 0004 tomorrow.
>
> Cool.
>
> I'd like to push
>
> bufmgr: Change BufferDesc.state to be a 64-bit atomic
> bufmgr: Implement buffer content locks independently of lwlocks
>
> pretty soon, so that we then can concentrate on
Other than the “lifting” comment, v11 LGTM. But that’s not a strong opinion. I
explained more above, if you consider that’s not a problem, I am totally fine.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
