Horton

Thread

Commits

  1. Switch Get[Local]BufferDescriptor() to use a signed value in input

  2. Fix MarkBufferDirtyHint() to not call GetBufferDescriptor() for local buffers

  3. Require share-exclusive lock to set hint bits and to flush

  1. GetBufferDescriptor() being called for local buffers from MarkBufferDirtyHint()

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2026-06-06T08:07:42Z

    Hi Andres,

82467f627bd478569de04f4a3f1993098e80c812 added MarkBufferDirtyHint()
which invokes GetBufferDescriptor() even for local buffers for which
id < 0. Since GetBufferDescriptor() declares id as uint32, -1 is
converted to a very large int32 value which is way larger than
NBuffers. Thus GetBufferDescriptor() may be returning something from
the BufferBlocks which probably has enough memory to accommodate that
memory access. But it's a bogus BufferDesc nevertheless. We are not
seeing any problem with this right now since MarkBufferDirtyHint()
uses the BufferDesc only when it's a shared buffer. Right fix is to
let that function handle local buffers first and then call
GetBufferDescriptor() as in the attached patch.

I caught this because of an Assertion added in GetBufferDescription()
in my shared buffer resizing patches. I think it's worth committing
that assertion and the related change to BufferManagerShmemInit()
separately from shared buffer resizing patches. Included those changes
in the attached patch as well.



-- 
Best Wishes,
Ashutosh Bapat

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

    Michael Paquier <michael@paquier.xyz> — 2026-06-10T03:40:38Z

    On Sat, Jun 06, 2026 at 01:37:42PM +0530, Ashutosh Bapat wrote:
> 82467f627bd478569de04f4a3f1993098e80c812 added MarkBufferDirtyHint()
> which invokes GetBufferDescriptor() even for local buffers for which
> id < 0. Since GetBufferDescriptor() declares id as uint32, -1 is
> converted to a very large int32 value which is way larger than
> NBuffers. Thus GetBufferDescriptor() may be returning something from
> the BufferBlocks which probably has enough memory to accommodate that
> memory access. But it's a bogus BufferDesc nevertheless. We are not
> seeing any problem with this right now since MarkBufferDirtyHint()
> uses the BufferDesc only when it's a shared buffer. Right fix is to
> let that function handle local buffers first and then call
> GetBufferDescriptor() as in the attached patch.

@@ -5831,8 +5831,6 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
 {
     BufferDesc *bufHdr;
 
-    bufHdr = GetBufferDescriptor(buffer - 1);
-
     if (!BufferIsValid(buffer))
         elog(ERROR, "bad buffer ID: %d", buffer);
 
@@ -5842,6 +5840,8 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
         return;
     }
 
+    bufHdr = GetBufferDescriptor(buffer - 1);

Yep, that's clearly wrong.  We are lucky that it does not blow up
today but that's a ticking bomb.  Even with that in mind, the result
leads to a non-defined behavior.

-    return &(BufferDescriptors[id]).bufferdesc;
+    BufferDesc *bdesc = &(BufferDescriptors[id]).bufferdesc;
+
+    Assert(bdesc->buf_id == id);
+
+    return bdesc; 
[...]
-        BufferDesc *buf = GetBufferDescriptor(i);
+        /* GetBufferDescriptor() expects buf_id to be set in the descriptor. */
+        BufferDesc *buf = &(BufferDescriptors[i]).bufferdesc;

I am not convinced by these two.  For the first one, the assertion is
a bound check in disguise, which could also use NBuffers as of an (id
< NBuffers).  With the inlining we care about the performance.  This
also forces the second change in ShmemInit(), which makes even less
sense once we think about the first assertion.

Will fix the first issue on HEAD, that's wrong.  Thanks for the
report.
--
Michael

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

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2026-06-10T10:50:18Z

    On Wed, Jun 10, 2026 at 9:10 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Sat, Jun 06, 2026 at 01:37:42PM +0530, Ashutosh Bapat wrote:
> > 82467f627bd478569de04f4a3f1993098e80c812 added MarkBufferDirtyHint()
> > which invokes GetBufferDescriptor() even for local buffers for which
> > id < 0. Since GetBufferDescriptor() declares id as uint32, -1 is
> > converted to a very large int32 value which is way larger than
> > NBuffers. Thus GetBufferDescriptor() may be returning something from
> > the BufferBlocks which probably has enough memory to accommodate that
> > memory access. But it's a bogus BufferDesc nevertheless. We are not
> > seeing any problem with this right now since MarkBufferDirtyHint()
> > uses the BufferDesc only when it's a shared buffer. Right fix is to
> > let that function handle local buffers first and then call
> > GetBufferDescriptor() as in the attached patch.
>
> @@ -5831,8 +5831,6 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
>  {
>      BufferDesc *bufHdr;
>
> -    bufHdr = GetBufferDescriptor(buffer - 1);
> -
>      if (!BufferIsValid(buffer))
>          elog(ERROR, "bad buffer ID: %d", buffer);
>
> @@ -5842,6 +5840,8 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
>          return;
>      }
>
> +    bufHdr = GetBufferDescriptor(buffer - 1);
>
> Yep, that's clearly wrong.  We are lucky that it does not blow up
> today but that's a ticking bomb.  Even with that in mind, the result
> leads to a non-defined behavior.
>
> -    return &(BufferDescriptors[id]).bufferdesc;
> +    BufferDesc *bdesc = &(BufferDescriptors[id]).bufferdesc;
> +
> +    Assert(bdesc->buf_id == id);
> +
> +    return bdesc;
> [...]
> -        BufferDesc *buf = GetBufferDescriptor(i);
> +        /* GetBufferDescriptor() expects buf_id to be set in the descriptor. */
> +        BufferDesc *buf = &(BufferDescriptors[i]).bufferdesc;
>
> I am not convinced by these two.  For the first one, the assertion is
> a bound check in disguise, which could also use NBuffers as of an (id
> < NBuffers).

Just to clarify, it's stronger than just a bound check.
BufferDesc::buf_id comment says
/*
* Buffer's index number (from 0). The field never changes after
* initialization, so does not need locking.
*/
>  With the inlining we care about the performance.  This
> also forces the second change in ShmemInit(), which makes even less
> sense once we think about the first assertion.
>

The field is writable and can be accidentally written to. Imagine a
bug *(wrong memory calculation) = <wrong buffer id> changing buf_id
without realising the change especially when <wrong buffer id> is
within 0 to NBuffers. There is no place where we check that the
assumption is true. But we use buf_id in a few critical places.
GetBufferDescriptor() is a good place for checking the assumption. I
agree that there's a small risk that it will make accessing the buffer
slower but only in Assert enabled builds which are anyway slower [1].

FWIW, I ran pgbench on my laptop with nproc = 16 with scale = 10. I
didn't find any performance difference

Without assertion
$ for i in 1 2 3 4 5 6; do pgbench -d postgres -T 300 -c 4 -j 4 | grep tps; done
starting vacuum...end.
tps = 1081.714940 (without initial connection time)
starting vacuum...end.
tps = 1026.242729 (without initial connection time)
starting vacuum...end.
tps = 1080.696554 (without initial connection time)
starting vacuum...end.
tps = 1092.211269 (without initial connection time)
starting vacuum...end.
tps = 1058.359337 (without initial connection time)
starting vacuum...end.
tps = 1087.718979 (without initial connection time)

With Assertion
$ for i in 1 2 3 4 5 6 ; do pgbench -d postgres -T 300 -c 4 -j 4 |
grep tps; done
starting vacuum...end.
tps = 1065.345204 (without initial connection time)
starting vacuum...end.
tps = 1088.642703 (without initial connection time)
starting vacuum...end.
tps = 1077.954350 (without initial connection time)
starting vacuum...end.
tps = 1091.662205 (without initial connection time)
starting vacuum...end.
tps = 1029.412347 (without initial connection time)
starting vacuum...end.
tps = 1086.912667 (without initial connection time)

Said that, I am fine, if we don't want to include the Assertion. I
will keep it in my shared buffer resizing patches where I have found
it useful. Maybe we will reconsider it with resizable shared buffers
context.

> Will fix the first issue on HEAD, that's wrong.  Thanks for the
> report.

Thanks.

[1] https://www.postgresql.org/message-id/CAExHW5tNMNQ5ennUM3QK%2BMQGw703M9T6z-LvBKKAcgpJ3KW14Q%40mail.gmail.com


--
Best Wishes,
Ashutosh Bapat

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

    Andres Freund <andres@anarazel.de> — 2026-06-10T14:36:22Z

    On 2026-06-10 12:40:38 +0900, Michael Paquier wrote:
> On Sat, Jun 06, 2026 at 01:37:42PM +0530, Ashutosh Bapat wrote:
> > 82467f627bd478569de04f4a3f1993098e80c812 added MarkBufferDirtyHint()
> > which invokes GetBufferDescriptor() even for local buffers for which
> > id < 0. Since GetBufferDescriptor() declares id as uint32, -1 is
> > converted to a very large int32 value which is way larger than
> > NBuffers. Thus GetBufferDescriptor() may be returning something from
> > the BufferBlocks which probably has enough memory to accommodate that
> > memory access. But it's a bogus BufferDesc nevertheless. We are not
> > seeing any problem with this right now since MarkBufferDirtyHint()
> > uses the BufferDesc only when it's a shared buffer. Right fix is to
> > let that function handle local buffers first and then call
> > GetBufferDescriptor() as in the attached patch.
> 
> @@ -5831,8 +5831,6 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
>  {
>      BufferDesc *bufHdr;
>  
> -    bufHdr = GetBufferDescriptor(buffer - 1);
> -
>      if (!BufferIsValid(buffer))
>          elog(ERROR, "bad buffer ID: %d", buffer);
>  
> @@ -5842,6 +5840,8 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
>          return;
>      }
>  
> +    bufHdr = GetBufferDescriptor(buffer - 1);
> 
> Yep, that's clearly wrong.  We are lucky that it does not blow up
> today but that's a ticking bomb.

I think it *should* blow up. It doesn't because we're lacking assertions in
GetBufferDescriptor(). But I don't think the assertions added in the patch are
quite right.

We can't trivially add the correct assertions, because somebody though it was
a good idea to give GetBufferDescriptor() a uint32 parameter, which seems
completely wrong to me.


> Even with that in mind, the result leads to a non-defined behavior.

I'm not sure it really does, but it's clearly wrong.

Greetings,

Andres Freund

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

    Michael Paquier <michael@paquier.xyz> — 2026-06-10T22:14:33Z

    On Wed, Jun 10, 2026 at 10:36:22AM -0400, Andres Freund wrote:
> I think it *should* blow up. It doesn't because we're lacking assertions in
> GetBufferDescriptor(). But I don't think the assertions added in the patch are
> quite right.
> 
> We can't trivially add the correct assertions, because somebody though it was
> a good idea to give GetBufferDescriptor() a uint32 parameter, which seems
> completely wrong to me.

This one is not as old as I expected: 3ac88fddd92c.  You're right that
switching that to be signed would be a correct first step forward.
--
Michael

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

    Ayush Tiwari <ayushtiwari.slg01@gmail.com> — 2026-06-29T07:59:09Z

    Hi,

On Thu, 11 Jun 2026 at 03:44, Michael Paquier <michael@paquier.xyz> wrote:

> On Wed, Jun 10, 2026 at 10:36:22AM -0400, Andres Freund wrote:
> > I think it *should* blow up. It doesn't because we're lacking assertions
> in
> > GetBufferDescriptor(). But I don't think the assertions added in the
> patch are
> > quite right.
> >
> > We can't trivially add the correct assertions, because somebody though
> it was
> > a good idea to give GetBufferDescriptor() a uint32 parameter, which seems
> > completely wrong to me.
>
> This one is not as old as I expected: 3ac88fddd92c.  You're right that
> switching that to be signed would be a correct first step forward.
>

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.

Thoughts?

Regards,
Ayush

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

    Ayush Tiwari <ayushtiwari.slg01@gmail.com> — 2026-06-30T19:17:46Z

    Hi,

On Mon, 29 Jun 2026 at 13:29, Ayush Tiwari <ayushtiwari.slg01@gmail.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
  2. leave NLocBuffer where it is and index LocalBufferDescriptors[]
directly
     in that loop.

Regards,
Ayush

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

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2026-07-01T05:33:51Z

    On Wed, Jul 1, 2026 at 12:47 AM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:
>
> Hi,
>
> On Mon, 29 Jun 2026 at 13:29, Ayush Tiwari <ayushtiwari.slg01@gmail.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

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

    Ayush Tiwari <ayushtiwari.slg01@gmail.com> — 2026-07-01T14:30:38Z

    Hi,

On Wed, 1 Jul 2026 at 11:04, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
wrote:

> On Wed, Jul 1, 2026 at 12:47 AM Ayush Tiwari
> <ayushtiwari.slg01@gmail.com> 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

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

    Michael Paquier <michael@paquier.xyz> — 2026-07-02T04:46:54Z

    On Wed, Jul 01, 2026 at 08:00:38PM +0530, Ayush Tiwari wrote:
> 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.

Catching up with the latest activity of this thread now that v20 has
opened up.  That's kind of nice.

I have been also reminded about BufferIsValid(), where we already have
similar assertions.  As a whole, that seems acceptable to me, and
using unsigned still does not make much sense here.

Any thoughts or objections regarding a HEAD-only change?
--
Michael

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

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2026-07-02T04:59:47Z

    On Thu, Jul 2, 2026 at 10:16 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Jul 01, 2026 at 08:00:38PM +0530, Ayush Tiwari wrote:
> > 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.
>
> Catching up with the latest activity of this thread now that v20 has
> opened up.  That's kind of nice.
>
> I have been also reminded about BufferIsValid(), where we already have
> similar assertions.  As a whole, that seems acceptable to me, and
> using unsigned still does not make much sense here.
>
> Any thoughts or objections regarding a HEAD-only change?

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.

-- 
Best Wishes,
Ashutosh Bapat

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

    Michael Paquier <michael@paquier.xyz> — 2026-07-02T23:21:01Z

    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

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

    Ayush Tiwari <ayushtiwari.slg01@gmail.com> — 2026-07-03T00:52:12Z

    Hi,

On Fri, 3 Jul 2026 at 04:51, Michael Paquier <michael@paquier.xyz> 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

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

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2026-07-03T02:47:22Z

    On Fri, Jul 3, 2026 at 4:51 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> 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.
>

Since you mentioned "HEAD-only" in your question "Any thoughts or
objections regarding a HEAD-only change?", it felt like you are fine
making this change but are debating whether to accept it only on HEAD
or backbranches as well. Sorry for the misunderstanding.

> 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.

Every code fragment, except the ones listed below, that uses
NLocBuffer is gated, eventually, by
if (LocalBufHash == NULL)
InitLocalBuffers();

So we will not access NLocBuffer unless LocalBufHash is initialized.
If we are accessing it in a way that leads to a crash, we may have a
subtle bug lurking somewhere.

An actual subtle problem does exist in check_temp_buffers.  If we set
NLocBuffer to a non-zero value when LocalBufHash is not initialized,
it's going to prohibit a change in the GUC. If LocalBufHash allocation
fails, probably user would like to try again by reducing NLocBuffer.
The prohibition makes that impossible.

The other such usage in hashbuild() is fine, since it will allocate
more space if at all.

CheckForLocalBufferLeaks() walks the local buffer descriptor array,
but it's already initialized when NLocBuffer is set.

Given that it's better not to move the assignment above just for the
sake of an assertion. However, I am worried if there is a crash which
can result because of that movement.

-- 
Best Wishes,
Ashutosh Bapat

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

    Michael Paquier <michael@paquier.xyz> — 2026-07-03T03:07:21Z

    On Fri, Jul 03, 2026 at 08:17:22AM +0530, Ashutosh Bapat wrote:
> An actual subtle problem does exist in check_temp_buffers.  If we set
> NLocBuffer to a non-zero value when LocalBufHash is not initialized,
> it's going to prohibit a change in the GUC. If LocalBufHash allocation
> fails, probably user would like to try again by reducing NLocBuffer.
> The prohibition makes that impossible.
> 
> The other such usage in hashbuild() is fine, since it will allocate
> more space if at all.
> 
> CheckForLocalBufferLeaks() walks the local buffer descriptor array,
> but it's already initialized when NLocBuffer is set.

Sure, now a lot of this code is also shaped with some internal static
routines, so I'm also slightly afraid of some refactorings.  It just
feels like in the long-term setting NLocBuffer earlier makes some of
the assumptions of the code a bit more brittle.  Not a bid deal,
still..
--
Michael

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

    Michael Paquier <michael@paquier.xyz> — 2026-07-03T03:08:03Z

    On Fri, Jul 03, 2026 at 06:22:12AM +0530, Ayush Tiwari wrote:
> 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.

Ah sorry, I have unfortunately missed your point upthread.  At least
we are on the same line.  :)

I have just applied your v3, let's close this thread.
--
Michael