Re: Changing shared_buffers without restart

On Thu, Sep 17, 2026 at 9:32 PM Palak Chaturvedi
<chaturvedipalak1911(at)gmail(dot)com> wrote:

> > I also tested the patches and did some more review.
> >
> > With shared_buffers=32MB and max_shared_buffers left at its default, parallel
> > workers fail to start, even without any resize:
> > FATAL: failed to initialize shared_buffers to 16384
> > CONTEXT: parallel worker
> >
> > RestoreGUCState() resets the GUC to its 128MB boot value, which exceeds
> > MaxNBuffers and fails the check hook.
>
> Reproduced this with shared_buffers=32MB and max_shared_buffers left at
> its default. Fixed in 0015 by allowing the temporary PGC_S_DEFAULT
> assignment only while InitializingParallelWorker is set.
>
> Also checked removing the last configured value. Allowing PGC_S_DEFAULT
> unconditionally accepts the 128MB default above the reserved 32MB
> maximum, followed by a PANIC on resize. The revised check rejects this
> reset.
>
> Added test 008 for both cases. All nine assertions pass, including
> launching two workers. Without the fix, it fails with the reported
> initialization error. It also fails with the unconditional
> default-source exception.
>

In the latest patchset this is 0015. I am wondering why does the
finalMaxNBuffers guard is not working OR why max_shared_buffers isn't
being reset. If max_shared_buffers also gets reset, it will default to
shared_buffers and the check will pass. This needs more investigation.
Please note that this is noted at
src/test/buffermgr/t/002_client_join_buffer_resize.pl:89, so a
separate test may not be needed.

> > There is also a performance issue in the grow path:
> > PGSharedMemoryEnsureAllocated() gets the whole structure's range, rather than
> > just the added range. Even a small increase makes MADV_POPULATE_WRITE walk the
> > existing range again, adding overhead for large buffer pools. Could we limit
> > this to the page-aligned [current_end, new_end), as in the shrink path?
>
> Changed this in 0013 to populate only the newly added pages. Also did
> the testing. For a 512MB to 528MB grow, seven untraced runs gave a median
> resize time of 332.7ms with the old range and 11.4ms with the new range.
> Repeated the optimized run after the control and got 11.4ms again.
>
> This was on Linux 6.17 with huge_pages=off, no swap and no concurrent
> workload. These are total resize times for this case. Existing pages
> that have been swapped out will be brought back when accessed. I have
> not measured the effect under memory pressure with swap enabled.
>

We mmap the shared memory with MAP_NORESERVE so swap is not reserved
for this segment. From your description I can't figure out whether the
performance problem is real or not. The reason it's coded like that is
purely to simplify code - it avoids tracking the current allocation
boundary and relies only on the current size of the structure. We
should fix it if there's visible performance problem. I think there's
also some hazard if we try to remove pages from protected range. That
may require this patch irrespective of the performance problem. But I
don't see that being mentioned in the commit message.

>
> Done:
>
> - 0011: Included the earlier pg_buffercache fix to process one buffer
> at a time in pg_buffercache_os_pages.

I will review this in detail, but the idea to process one buffer at a
time looks promising. If we could avoid performance degradation in the
NUMA path that would be great.

>
> - 0012: Recompute MaxProportionalPins when a backend processes a resize
> barrier, so the pin limit follows the new pool size.

This looks ok too. Recompute in the function name does look misleading
since the same function is used to compute MaxProportionalPins the
first time and later. Did you check whether changing the value of
MaxProportionalPins in a barrier is safe?

>
> - 0013: Retained the corrected MADV_REMOVE shrink range and included
> the grow-range change discussed above. Also included the configure
> declaration checks for the madvise constants.

configure.ac has these changes. Do we need to include the
corresponding changes to configure in the patch?

>
> - 0014: Check BM_TAG_VALID during eviction. A buffer can already have
> a lookup-table entry while its read is still in progress, so checking
> only whether its contents are valid can miss it. Recheck under the
> header lock and roll back if eviction fails. Test 006 checks rollback,
> reader completion and successful retry.
> 003 already covers the fully-valid pinned-buffer case.

EvictExtraBuffers is inspired from other Evict*Unpinned* functions in
that file. Those functions do not check for this flag? Do we need to
handle it there as well?

>
> - 0016: Covered the TODO to document unsupported platforms. The
> documentation now says that the resize function reports an error
> when resizable shared memory is unavailable. Also repaired the
> garbled paragraph and the typo.
> See doc/src/sgml/func/func-admin.sgml:163.

- <!-- TODO: Document behaviour when the function is called on
platforms that do not support resizable shared memory -->
- linkend="functions-admin-signal-table"/> send control signals to
- other server processes. Use of these functions is restricted to
- superusers by default but access may be granted to others using
- <command>GRANT</command>, with noted exceptions.
- </para>
+ If <xref linkend="guc-have-resizable-shmem"/> is
<literal>off</literal>,
+ this function reports an error instead of resizing the buffer pool, and
+ the pending value of <varname>shared_buffers</varname> takes
effect only
+ after the server is restarted.
+ </para>

Have you accidentally removed some other function's description?

>
> - 0016: Covered the numeric memory-overhead example TODO. Verified with
> max_shared_buffers=1GB and 8kB blocks on the 64-bit build. The lookup
> table and checkpoint sort array together allocate 9,976,780 bytes,
> about 9.5MB. Got the same result with shared_buffers=32MB and 128MB.
> See doc/src/sgml/config.sgml:1875.

Thanks.

>
> - 0017: Covered the TODO to validate hints inside ReadRecentBuffer().
> A buffer number saved before shrink may be outside the new pool.
> The function now rejects it before accessing the descriptor,
> allowing the caller to do a normal lookup.
> See src/backend/storage/buffer/bufmgr.c:843.

XLogReadBufferExtended() still checks for InvalidBuffer and the other
caller invalidate_one_block remains unchanged. Am I missing something.

>
> - 0017: Covered the TODO to check pg_shmem_allocations after rollback.
> Test 001 now compares allocation metadata for all five buffer-manager
> structures instead of checking only buffer counts. Also disabled
> huge pages so rounding does not hide the small allocation changes.
> All 414 assertions pass. This checks allocation metadata, not whether
> physical memory was released.
> See src/test/buffermgr/t/001_resize_fault_tolerance.pl:290.
> The snapshot helper is at line 48; huge_pages=off is at line 29.
>

Even with huge_pages = off, the previous allocations should match
after resize rolls back, right?

> - 0018: Included the earlier SIGHUP test-ordering fix from
> v20260817-0009. Start the resizer after configuration reload so the
> expansion-failure test does not attempt resize with the old target.

Thanks a lot for this fix. Included in my patch set now.

>
> - 0019: Included the earlier mprotect diagnostic fix from
> v20260908-0011. The messages distinguish failure to make the active
> range writable from failure to protect the reserved tail.
>

I think we will come to these fine tuning changes as we are nearer to
the finalization patch. Is this something causing inconvenience during
testing or review?

> Not done:
>
> - The variable-naming changes raised earlier are still pending.

These too should probably wait till we have a wider agreement on the
high level design.

>
> - Direct stale-hint coverage for 0017. The code check is included, but
> there is no test that keeps a buffer-number hint across a shrink
> which removes that buffer slot. Such a test should check that the
> old hint is rejected and the caller finds the page by normal lookup.
> The rollback tests do not exercise this case.
>

I couldn't easily find the discussion that leads to this. Can you
please elaborate?

> - SHOW shared_buffers still includes the pending target, so its output
> cannot be passed to pg_size_bytes(). Should SHOW return a parseable
> size and leave the pending target to pg_get_buffer_resize_status()?
>

pg_size_bytes() would fail only if there's "pending" size in the
output, otherwise it should succeed. But if a size change is pending
then SHOWing just one of either the current NBuffers or the pending
size is going to be misleading. SHOW is not supposed to provide the
value of the GUC, not the size of the buffer pool. Maybe we will just
report the size of buffer pool or just the pending size ultimately,
but I would wait for a wider opinion before actually making that
change.

> - A failed grow may leave some pages allocated in the unused range.
> MADV_POPULATE_WRITE can fail after populating part of the range, and
> making it inaccessible again does not free those pages. I have not
> tested this failure case or added cleanup for it.

Is that possible? The madvise() documentation does not mention that possibility.

--
Best Wishes,
Ashutosh Bapat