Hi, On Fri, 3 Jul 2026 at 04:51, Michael Paquier <[email protected]> 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
v3-0001-Make-buffer-descriptor-accessors-take-signed-int.patch
Description: Binary data
