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

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