Re: GetBufferDescriptor() being called for local buffers from MarkBufferDirtyHint()

Hi,

On Fri, 3 Jul 2026 at 04:51, Michael Paquier <michael(at)paquier(dot)xyz> wrote:

>
> Anyway, while putting my eyes into all that in details, I saw one
> problem and one potential gap:
> - The problem. The change for local buffers is actually incorrect,
> where this patch decides to set NLocBuffer earlier. If the hash
> allocation fails on ERROR, we would track an incorrect state in
> memory, most likely crashing later if attempting a local buffer
> lookup.
>

You're right that establishing NLocBuffer before the local buffers
are initialized is not correct ig.

> - The potential gap: in freelist.c, ClockSweepTick() returns a uint32
> to identify a buffer ID. This would not match with the new definition
> of GetBufferDescriptor() due to the call in StrategyGetBuffer(). It
> does not matter in practice as the returned value is a modulo of
> NBuffers, so we'll always be in range. Switching ClockSweepTick() to
> a int would be incorrect to me, as in theory the counter to go past
> INT_MAX (unlikely, okay, still worth mentioning). It's OK to discard
> this one, just wanted to mention it.
>

Yeah I had mentioned this upthread, this is the only place returning
unit32 but with modulo.

> Ashutosh's argument was about shared buffers originally, not local
> buffers, so I'd suggest to reduce the change so as we make
> GetBufferDescriptor() and GetLocalBufferDescriptor() use signed ints,
> but only keep the assertion for shared buffers with NBuffers, which is
> safer due to the GUC value enforcement that happens earlier than the
> shmem initialization. That should be enough to address the original
> complaint, as well as enough for the case of his patch to make
> shared_buffers SIGHUP-able.
>

Makes sense.

v3 does what you suggested: both GetBufferDescriptor() and
GetLocalBufferDescriptor() now take a signed int, but only
GetBufferDescriptor()
keeps the bounds assertion (id >= 0 && id < NBuffers). I need to
spend some more time on the GetLocalBufferDescriptor function.

Regards,
Ayush