Hi, On Wed, 1 Jul 2026 at 11:04, Ashutosh Bapat <[email protected]> wrote:
> On Wed, Jul 1, 2026 at 12:47 AM Ayush Tiwari > <[email protected]> wrote: > > 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. > Thanks for looking! This matches my analysis as well. > > > 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. > Ahh I see, good to know it was already considered. 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. > I'm attaching a patch with option 1, ideally I would not want to extend the assertion, but if the option is not feasible for some reason, we'll have to go this way. v2 attached sets NLocBuffer = nbufs before the initialization loop, so the loop can keep using GetLocalBufferDescriptor(), and drops the now-redundant assignment at the end. Regards, Ayush
v2-0001-Make-buffer-descriptor-accessors-take-signed-int.patch
Description: Binary data
