On Thu, Jul 02, 2026 at 10:29:47AM +0530, Ashutosh Bapat wrote: > We usually do not backport changes to function definitions or new > assertions, unless they are critical to the fix. That is not the case > here. The functions have been in this state for a couple of releases > and we haven't heard any complaints. Overall HEAD-only is fine with > me. > > If required we can always backport, the functions are static so they > won't cause an ABI break. The Assertions are applicable in those > branches as well.
I've never mentioned a backpatch, FWIW. 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. - 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. 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. -- Michael
signature.asc
Description: PGP signature
