Re: Changing shared_buffers without restart

Hi Yuhang,

Thanks for the detailed review of v20260817. I went through
each finding against the code. Responses below, and one fix
patch (0010) attached.

On Thu, 20 Aug 2026 at 16:43, Yuhang Qiu <iamqyh(at)gmail(dot)com> wrote:
>
> Hi Ashutosh,
>
> I reviewed the v20260817 patches, and here is what I found:
>
> 0003 / 0004:
>
> BufTableGetContents() holds all mapping partition locks for the whole scan,
> with no CHECK_FOR_INTERRUPTS. pg_buffercache_lookup_table is a view whose name
> ends in "table", which is ambiguous. What about pg_buffercache_mappings?

Good catch on the missing CHECK_FOR_INTERRUPTS. The locks themselves
are there to give a consistent view of the mapping table during the
scan, so I don't think we can simply drop them, but you're right that
a long scan with no interrupt check isn't great. Would add it as a TODO.

On the view name, I'd like to note it down and come back to it a bit
later. Right now the discussion on this patch is still fairly
high-level, so I think naming is better revisited once the shape
settles.

>
> With a resize pending, pg_settings.setting reads "16384 (pending: 32768)",
> could break pg_size_bytes(current_setting('shared_buffers')). Maybe we need a
> new GUC rather than a composite one.
>

You're right, and it does break rather than just might. I tried it:

ERROR: invalid size: "128MB (pending: 256MB)"
DETAIL: Invalid size unit: "MB (pending: 256MB)".

That happens for the whole window between the reload and the resize
finishing, so anything parsing that value would be affected.

Whether we go with a separate GUC or keep the composite string feels
like a design call that's better made when the patch is closer to
committable, so I'd suggest holding it for now. I just wanted it on
record that the breakage is real.

> 0006:
>
> configure hasn't been regenerated. The feature might not be compiled by default.

Need little more information on this.
>
> Shrinking PANICs, whenever max_shared_buffers > shared_buffers.
> The range handed to madvise(MADV_REMOVE) in ShmemResizeStruct() ends at
> maximum_size rather than at the current size, so it covers the PROT_NONE tail
> and fails with EACCES.
>

I agree with you on the range. It should stop at result->size so we
only free the pages actually being shrunk away, rather than running on
into the PROT_NONE tail.

One small thing so we're sure we're looking at the same problem: it
seems whether this actually PANICs depends on the kernel. On my box
(Linux 6.17, huge_pages=off) those over-wide MADV_REMOVE calls all
return 0, so I don't hit the PANIC locally. The range is still wrong
either way, so this doesn't change the conclusion.

> There are now six read-only shared_memory_* values, and the names are getting
> long. A function might fit better than that many GUCs.
> shared_memory_size_in_huge_pages is gone, it might break compatibility.
>

Both of these seem reasonable to me. Same as the naming point earlier,
I'd like to hold them until we're closer to commit.

> MADV_POPULATE_WRITE requires a new OS kernel version. #ifdef is needed in
> PGSharedMemoryEnsureAllocated.
>

I think the compile-time guard is actually already there, just
indirectly. The madvise call sits inside that function's existing
#ifndef HAVE_RESIZABLE_SHMEM block, and HAVE_RESIZABLE_SHMEM itself
requires HAVE_DECL_MADV_POPULATE_WRITE, so an extra #ifdef would end
up redundant.

> "could not protect shared memory" is emitted from two places, so it's not
> possible to tell which one failed.
>

Partly covered already, I think. The caller does report which
structure failed, so that part reaches the log. What's missing is just
which of the two mprotect calls it was, and those two fail for fairly
different reasons.

I've attached 0011 which gives each call site its own message, "could
not make shared memory read-write" and "could not make reserved shared
memory inaccessible".

> 0007:
>
> EvictExtraBuffers() checks BM_VALID, but BM_TAG_VALID is the flag that means
> there's a mapping table entry, so buffers with IO in flight are skipped by the
> precheck.
>
Agreed, and thanks for spotting this one. BM_TAG_VALID goes on when
BufferAlloc publishes the tag into the mapping table, while BM_VALID
only goes on after the read finishes. So a buffer sitting between the
two has a live hash entry that the current precheck walks past, and if
it happens to be above the new target when the shrink runs, that entry
gets orphaned.

I did try to reproduce it, including widening the window artificially,
but couldn't catch it. It seems to need AIO in the mix, so it would be
good to discuss this one a bit more.

> "shared buffer resizing to %d buffers failed" doesn't say why it failed.
>
Here I'd gently push back. I think that line is only meant as the
final summary, and the actual reason gets logged by whichever phase
failed, right before it. For example:

WARNING: failed to expand buffer pool structures
WARNING: shared buffer resizing to 25600 buffers failed

So the information is there, just on the previous line.

> Some values like MaxProportionalPins aren't recomputed on resize.
>

Confirmed, and this is what the attached 0010 fixes.
InitBufferManagerAccess() computes it once at startup and nothing
revisits it afterwards, so after a shrink every backend keeps using a
cap sized for the old pool. The patch moves the computation into
RecomputeMaxProportionalPins() and calls it from
ProcessBarrierBufferPoolSize(), so each backend refreshes as it takes
in the new pool size.

> 001_resize_fault_tolerance.pl never reaches madvise(): with
> max_shared_buffers = 32 buffers and huge pages still on for the TAP cluster,
> the range rounds away at 2MB granularity, so everything passes with nothing
> freed. That is also why the shrink problem above goes unnoticed.
>

Confirmed, and there's one more detail that makes it worse. The
buffermgr_test.conf that sets huge_pages=off is wired in as
TEMP_CONFIG for the regress target only, so the TAP clusters never
pick it up, and 001 doesn't set it itself either.

Could you share the error you saw, or how to reproduce it?
HugePages_Total is 0 on my machine, so the shrink does reach madvise
here and 001 still passes for me, and I'd like to be sure I'm seeing
the same thing you are.

Thanks again for going through the patch so carefully.

Thanks,
Palak