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?
> 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 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
Require share-exclusive lock to set hint bits and to flush
and then subsequently on
WIP: bufmgr: Don't copy pages while writing out
as there are other patches that have this work as a dependency...
Greetings,
Andres Freund
