Re: Changing shared_buffers without restart

Hi folks,

Attached is v20261017. 0001-0010 are Ashutosh's commits, unchanged.
0011-0023 are mine. Compared with v20260917:
- 0015 is rewritten: shared_buffers is now capped at
max_shared_buffers instead of allowing the boot-value reset only in
parallel workers.
- 0014's test 006 is rewritten to place the in-progress read reliably.
- 0018, 0020 and 0022 are new. 0021 is the test_shmem CI fix from
August.
- 0019 and 0023 are the old 0018 and 0019.

On Tue, 22 Sept 2026 at 12:32, Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>
> 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.

The guard works; it is what rejects the value. RestoreGUCState() skips
PGC_POSTMASTER GUCs (can_skip_gucvar()), so max_shared_buffers is never
reset in the worker, while shared_buffers (PGC_SIGHUP) is reset to its
128MB boot value. Removing shared_buffers from the configuration hits
the same reset outside parallel workers.

0015 is rewritten to cap instead of reject: shared_buffers stores the
request, and the pool size is min(request, max_shared_buffers). A
capped value is logged as a WARNING at startup and on reload. Do you
prefer this, or should startup still fail when shared_buffers exceeds
an explicit max_shared_buffers?

Agreed about 002. It now runs with a parallel query and fails without
0015. 008 is reduced to the configuration-removal case, which 002 does
not cover.

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

The performance problem is real. Growing the pool by 16MB with
pg_resize_shared_buffers() took 174ms with the old range and 13ms with
the new one from a 256MB pool, and 1180ms vs 12ms from a 2GB pool
(median of seven runs, release build, huge_pages = off). With the old
range the cost grows with the pool size; with the new one it depends
only on the increment.

The hazard is real too. On shrink, MADV_REMOVE reached into the
PROT_NONE tail and failed with EACCES on Linux 5.10, causing a PANIC.
The 0013 commit message now says so.

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

Not yet. The NUMA path is 7-10% slower because of one
pg_numa_query_pages() call per buffer. Batching would fix it; listed as
open.

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

Renamed to UpdateMaxProportionalPins(). It is safe: the value is
process-local, the barrier is absorbed in CHECK_FOR_INTERRUPTS(), and
callers read the limit afresh for each batch.

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

Yes. Without it, autoconf builds never define HAVE_RESIZABLE_SHMEM and
all resize tests skip.

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

No. Their buffers stay in the pool, so a skipped in-flight buffer is
harmless there. EvictExtraBuffers() removes the slot.

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

No. Those lines were a stray copy of the opening of the
signal-functions section, left inside the pg_resize_shared_buffers()
entry. 0016 removes the copy; the original paragraph is unchanged.

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

Removed from XLogReadBufferExtended(). invalidate_one_block() is left
alone because its hint comes from PrefetchBuffer() just before and
cannot be stale.

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

Yes. huge_pages = off lets the test notice when they don't: with 2MB
pages the small structures in 001 round to the same size.

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

Kept as 0019 until your next posting.

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

No. It is the last patch (0023) and can be dropped.

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

Agreed.

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

It is your TODO above ReadRecentBuffer(). A hint saved by the recovery
prefetcher can name a slot removed by a shrink before redo uses it.
0017 checks the range in ReadRecentBuffer(). There is no test for it
yet.

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

Agreed.

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

Yes, madvise(2) says so under MADV_POPULATE_WRITE: "If
MADV_POPULATE_WRITE fails, some page tables might have been populated."
https://man7.org/linux/man-pages/man2/madvise.2.html
Not tested or handled yet.

On Tue, 22 Sept 2026 at 11:45, Yuhang Qiu <iamqyh(at)gmail(dot)com> wrote:
> Thanks for the update. 0013 and 0015 LGTM.

Thanks. 0015 is rewritten in this version, so it needs another look:
it now caps shared_buffers at max_shared_buffers (see above).

> - pg_resize_shared_buffers() lacks a permission check, although the
> documentation says it is restricted to superusers.

Fixed in 0018. EXECUTE is revoked from PUBLIC, and the function also
checks superuser(), so a GRANT does not extend it. The regression test
covers both cases.

> - In test 006, evicting the low-numbered buffers leaves those slots
> available for reuse, so the victim may still be below the shrink target.
> The test can therefore bail out before attempting the shrink. What would
> be a reliable way to arrange an in-progress read above the target?

Right, the test cannot control which buffer the read lands in. 006 now
checks which buffer the stalled read actually got. If that buffer is 16
or below, which no shrink can remove, the test lets the read finish and
tries again. Once the read is in a removable buffer, the test shrinks
the pool to just below that buffer, so the shrink always has to deal
with it. The test fails without the fix in 0014.

Other changes in this version:
- 0020: a concurrent resize is now reported as a WARNING, so every
failed resize states its reason (Yuhang, 2026-08-20).
- 0022: adds max_shared_buffers to postgresql.conf.sample;
test_misc/003_check_guc failed without it.

Testing: each patch builds without warnings, and buffermgr,
test_shmem, test_aio and pg_buffercache pass at every patch. On the
full set, recovery (54/54), test_misc and the 015 stress test pass.

Still open:
- naming, including the pg_buffercache_lookup_table view
- SHOW shared_buffers format
- BufTableGetContents() holds all mapping partition locks for the
whole scan
- the six shared_memory_* GUCs; shared_memory_size_in_huge_pages is
removed
- NUMA overhead of 0011
- test for a stale ReadRecentBuffer() hint
- cleanup after a partially failed MADV_POPULATE_WRITE

Thanks,
Palak