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

On Wed, Jul 1, 2026 at 12:47 AM Ayush Tiwari
<ayushtiwari(dot)slg01(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Mon, 29 Jun 2026 at 13:29, Ayush Tiwari <ayushtiwari(dot)slg01(at)gmail(dot)com> wrote:
>>
>>
>> Given the discussion here, I tried making the buffer descriptor accessors take
>> a signed index and adding range assertions.
>>
>> Most callers already pass signed buffer indexes derived from Buffer values
>> (e.g. buffer - 1, or -buffer - 1 for local buffers). The only caller I found
>> that passes an unsigned value to GetBufferDescriptor() is ClockSweepTick(), and
>> that normalizes the value before returning it, so it should still be less than
>> NBuffers.
>>
>> The change looked fairly straightforward to me, unless I'm missing something.
>> I also made the same change for GetLocalBufferDescriptor(), since it has the
>> same uint32 signature and takes local buffer indexes.
>
>
> cfbot flagged v1 in an assertion-enabled (cassert) build: anything that uses a
> temporary table hits the new assertion in GetLocalBufferDescriptor():
>
> TRAP: failed Assert("id >= 0 && id < NLocBuffer"), buf_internals.h
> GetLocalBufferDescriptor
> InitLocalBuffers
> ExtendBufferedRelLocal -> ... -> heap_insert
>
> It looks like InitLocalBuffers() initializes the descriptors in a loop before
> it sets NLocBuffer, so during that loop the assert sees NLocBuffer == 0. The
> access itself seems fine, since the array is already allocated with
> num_temp_buffers entries; it's just that this is the one place that runs
> before the bound the assert checks against is set. (The shared path doesn't
> run into this, since NBuffers is already set when BufferManagerShmemInit()
> does the equivalent loop.)
>
> I'm not sure which way would be preferable here if are adding assert for
> LocalBufferDescriptor too:
>
> 1. set NLocBuffer = nbufs before the init loop, so the loop can keep using
> GetLocalBufferDescriptor(); or

This looks safe for me. The callers of InitLocalBuffers rely on
LocalBufferHash to decide whether or not local buffers have been
initialized. Further local buffers are local to the backend,
concurrent access is not possible. So, even if we initialize
NLocalBuffer before actually allocating the buffers, nobody is going
to access the local buffers before they are fully initialized.
LocalBufferHash still serves as a definitive condition to decide
whether or not local buffers are initialized.

> 2. leave NLocBuffer where it is and index LocalBufferDescriptors[] directly
> in that loop.

I had suggested this in the context of shared buffers and it was shot
down by Michael. So probably the same argument applies here.

If 1 is not possible for some reason, you could augment your assertion
as Assert(id >= 0 && (!LocalHashBuffers || id < NLocBuffer)) or
separate the two operands of && into separate assertions.

--
Best Wishes,
Ashutosh Bapat