Re: Changing shared_buffers without restart

On Fri, Nov 14, 2025 at 5:23 PM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>
> Hi,
> PFA new patchset with some TODOs from previous email addressed:
>
> On Mon, Oct 13, 2025 at 9:28 PM Ashutosh Bapat
> <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> > 1. New backends join while the synchronization is going on.
>
> Done. Explained the solution below in detail.
>
> > An existing backend exiting.
>
> Not tested specifically, but should work.
>
> > 2. Failure or crash in the backend which is executing pg_resize_buffer_pool()
>
> still a TODO
>
> > 3. Fix crashes in the tests.
>
> core regression passes, pg_buffercache regression tests pass and the
> tests for buffer resizing pass most of the time. So far I have seen
> two issues
> 1. An assertion from AIO worker - which happened only once and I
> couldn't reproduce again. Need to study interaction of AIO worker with
> buffer resizing.
> 2. checkpointer crashes - which is one of the TODOs listed below.
> 3. Also there's an shared memory id related failure, which I don't
> understand but happen more frequently than the first one. Need to look
> into that.
>
> > go through Tomas's detailed comments and address those
> > which still apply.
>
> Still a TODO. But since many of those patches are revised heavily, I
> think many of the comments may have been addressed, some may not apply
> anymore.

Thanks a lot Tomas for these review comments and summary of the
discussion as of your response. Sorry, it took so long to fully
respond to this email, but I wanted the patches to be in a reasonable
shape before responding. It's been a while since I have posted the
last patchset. Hence attaching patchset along with responses to your
comments as well as the earlier comments that you mentioned in your
response. Dmitry has responded to some parts of these emails already
but I am responding to all the comments in the context of the latest
implementation which has been revised heavily since his responses. The
latest design and UI is described in details in [1]. Please note that
that email also responds to some earlier comments from Andres and
others. I have not repeated those responses here.

The earlier patches did not build in EXEC_BACKEND mode. The attached
patches build in EXEC_BACKEND mode and also the regression tests pass
in that build. I haven't tried running whole test suite though.

>
> I agree it'd be useful to be able to resize shared buffers, without
> having to restart the instance (which is obviously very disruptive). So
> if we can make this work reliably, with reasonable trade offs (both on
> the backends, and also the risks/complexity introduced by the feature).

Agreed. That's the goal - to make it work reliably with reasonable
trade-offs. Adding some details to this goal as follows:

1. Performance
--------------
a. The impact of resizing on the performance of the concurrent
database operations should be reasonable and acceptable.
b. The code changes should not cause significant performance
degradation during normal operations

Palak Chaturvedi has already done some benchmarking. I am listing her
findings here
1. The code changes do not have any noticeable performance degradation
during normal operations.
2. Resizing itself is also reasonably fast and with very minimal,
almost unnoticeable, performance impact on the concurrent
transactions.

We will share the benchmarks once the code is cleaner and more complete stable.

2. Reliability
--------------
We are adding tests to make sure that resizing does not introduce any
crashes, hazards or data corruption. Some tests are already part of
the patch set.

>
> I'm far from an expert on mmap() and similar low-level stuff, but the
> current appproach (reserving a big chunk of shared memory and slicing
> it by mmap() into smaller segments) seems reasonable.

The latest patches, we reserve address space by mmap and manage memory
within that address space using file backed memory.

>
> But I'm getting a bit lost in how exactly this interacts with things
> like overcommit, system memory accounting / OOM killer and this sort of
> stuff. I went through the thread and it seems to me the reserve+map
> approach works OK in this regard (and the messages on linux-mm seem to
> confirm this). But this information is scattered over many messages and
> it's hard to say for sure, because some of this might be relevant for
> an earlier approach, or a subtly different variant of it.

Looking at the documentation and the messages on linux-mm, it seems
that the mmap with MAP_NORESERVE approach works well with overcommit,
system memory accounting and OOM killer. I plan to add tests verify
these aspects, so that any changes in the future do not cause any
regressions. I am looking for ways to write a small test program for
the same, which we can add to our test battery. But no success so far.
Any idea?

>
> A similar question is portability. The comments and commit messages
> seem to suggest most of this is linux-specific, and other platforms just
> don't have these capabilities. But there's a bunch of messages (mostly
> by Thomas Munro) that hint FreeBSD might be capable of this too, even if
> to some limited extent. And possibly even Windows/EXEC_BACKEND, although
> that seems much trickier.
>
> FWIW I think it's perfectly fine to only support resizing on selected
> platforms, especially considering Linux is the most widely used system
> for running Postgres. We still need to be able to build/run on other
> systems, of course. And maybe it'd be good to be able to disable this
> even on Linux, if that eliminates some overhead and/or risks for people
> who don't need the feature. Just a thought.

+1. Plan is to make it work on Linux first and disable it on the other
platforms. I think it's a good idea to be able to disable it at initdb
time say. But I am not yet sure if that's going to avoid any overhead
or risks. I haven't yet wrapped my head around the platform dependency
completely.

>
> Anyway, my main point is that this information is important, but very
> scattered over the thread. It's a bit foolish to expect everyone who
> wants to do a review to read the whole thread (which will inevitably
> grow longer over time), and assemble all these pieces again an again,
> following all the changes in the design etc. Few people will get over
> that hurdle, IMHO.
>
> So I think it'd be very helpful to write a README, explaining the
> currnent design/approach, and summarizing all these aspects in a single
> place. Including things like portability, interaction with the OS
> accounting, OOM killer, this kind of stuff. Some of this stuff may be
> already mentioned in code comments, but you it's hard to find those.
>
> Especially worth documenting are the states the processes need to go
> through (using the barriers), and the transacitons between them (i.e.
> what is allowed in each phase, what blocks can be visible, etc.).
>

That's a great idea. There is already a README in
src/backend/storage/buffer/, I have added a section of buffer pool
resizing. The section has pointers to relevant code comments. I will
revise this to be as self sufficient as possible as the patches
mature. The code managing the shared memory is scattered across
backend/portability, storage/ipc etc. It's slightly troublesome to
find one good place to add a README which explains everything.

>
> I'll go over some higher-level items first, and then over some comments
> for individual patches.
>
>
> 1) no user docs
>
> There are no user .sgml docs, and maybe it's time to write some,
> explaining how to use this thing - how to configure it, how to trigger
> the resizing, etc. It took me a while to realize I need to do ALTER
> SYSTEM + pg_reload_conf() to kick this off.

Latest patches have updated config.sgml and func-admin.sgml to
document the functionality. Please review them.

>
> It should also document the user-visible limitations, e.g. what activity
> is blocked during the resizing, etc.
>

We are still figuring this out. But we will document the limitations
in func-admin.sgml. Right now there are no limitations and my
intention is to keep the limitations as limited as possible.

>
> 2) pending GUC changes
>
> I'm somewhat skeptical about the GUC approach. I don't think it was
> designed with this kind of use case in mind, and so I think it's quite
> likely it won't be able to handle it well.
>
> For example, there's almost no validation of the values, so how do you
> ensure the new value makes sense? Because if it doesn't, it can easily
> crash the system (I've seen such crashes repeatedly, I'll get to that).
> Sure, you may do ALTER SYSTEM to set shared_buffers to nonsense and it
> won't start after restart/reboot, but crashing an instance is maybe a
> little bit more annoying.
>
> Let's say we did the ALTER SYSTEM + pg_reload_conf(), and it gets stuck
> waiting on something (can't evict a buffer or something). How do you
> cancel it, when the change is already written to the .auto.conf file?
> Can you simply do ALTER SYSTEM + pg_reload_conf() again?
>
> It also seems a bit strange that the "switch" gets to be be driven by a
> randomly selected backend (unless I'm misunderstanding this bit). It
> seems to be true for the buffer eviction during shrinking, at least.
>
> Perhaps this should be a separate utility command, or maybe even just
> a new ALTER SYSTEM variant? Or even just a function, similar to what
> the "online checksums" patch did, possibly combined with a bgworder
> (but probably not needed, there are no db-specific tasks to do).
>

As documented in func-admin.sgml, and config.sgml, resizing the buffer
pool requires ALTER SYSTEM + pg_reload_conf() followed by
pg_resize_shared_buffers(). There is no pending flag, no arbitrary
backend driving the process. The validation, failures are handled by
pg_resize_shared_buffers(). The backend where
pg_resize_shared_buffers() is called is responsible for driving the
resizing process. I think, the new implementation takes care of all
the concerns mentioned above.

>
> 3) max_available_memory
>
> Speaking of GUCs, I dislike how max_available_memory works. It seems a
> bit backwards to me. I mean, we're specifying shared_buffers (and some
> other parameters), and the system calculates the amount of shared memory
> needed. But the limit determines the total limit?
>
> I think the GUC should specify the maximum shared_buffers we want to
> allow, and then we'd work out the total to pre-allocate? Considering
> we're only allowing to resize shared_buffers, that should be pretty
> trivial. Yes, it might happen that the "total limit" happens to exceed
> the available memory or something, but we already have the problem
> with shared_buffers. Seems fine if we explain this in the docs, and
> perhaps print the calculated memory limit on start.
>
> In any case, we should not allow setting a value that ends up
> overflowing the internal reserved space. It's true we don't have a good
> way to do checks for GUcs, but it's a bit silly to crash because of
> hitting some non-obvious internal limit that we necessarily know about.
>
> Maybe this is a reason why GUC hooks are not a good way to set this.

Latest patches introduce max_shared_buffers which specifies the
maximum shared buffers allowed. max_available_memory, that some
previous patchsets had, has been removed. This GUC is mentioned in the
documentation changes.

One relatively small thing to think about is what do we call
shared_buffers GUC - it's not SIGHUP per say since it requires a
function to be called after the reload but it's not POSTMASTER either
since restart is not required. I think we need a new PGC_ for it but
haven't yet thought of a good name. Do you have any suggestions?

>
>
> 4) SHMEM_RESIZE_RATIO
>
> The SHMEM_RESIZE_RATIO thing seems a bit strange too. There's no way
> these ratios can make sense. For example, BLCKSZ is 8192 but the buffer
> descriptor is 64B. That's 128x difference, but the ratios says 0.6 and
> 0.1, so 6x. Sure, we'll actually allocate only the memory we need, and
> the rest is only "reserved".
>
> However, that just makes the max_available_memory a bit misleading,
> because you can't ever use it. You can use the 60% for shared buffers
> (which is not mentioned anywhere, and good luck not overflowing that,
> as it's never checked), but those smaller regions are guaranteed to be
> mostly unused. Unfortunate.
>
> And it's not just a matter of fixing those ratios, because then someone
> rebuilds with 32kB blocks and you're in the same situation.
>
> Moreover, all of the above is for mappings sized based on NBuffers. But
> if we allocate 10% for MAIN_SHMEM_SEGMENT, won't that be a problem the
> moment someone increases of max_connection, max_locks_per_transaction
> and possibly some other stuff?
>

+1. Fixed. max_shared_buffers only deals with the shared buffers.

>
> 5) no tests
>
> I mentioned no "user docs", but the patch has 0 tests too. Which seems
> a bit strange for a patch of this age.
>
> A really serious part of the patch series seems to be the coordination
> of processes when going through the phases, enforced by the barriers.
> This seems like a perfect match for testing using injection points, and
> I know we did something like this in the online checksums patch, which
> needs to coordinate processes in a similar way.
>
> But even just a simple TAP test that does a bunch of (random?) resizes
> while running a pgbench seem better than no tests. (That's what I did
> manually, and it crashed right away.)
>
> There's a lot more stuff to test here, I think. Idle sessions with
> buffers pinned by open cursors, multiple backends doing ALTER SYSTEM
> + pg_reload_conf concurrently, other kinds of failures.
>

+1. The latest patches already have a few tests but we are adding more
tests to cover different scenarios.

>
> 6) SIGBUS failures
>
> As mentioned, I did some simple tests with shrink/resize with a pgbench
> in the background, and it almost immediately crashed for me :-( With a
> SIGBUS, which I think is fairly rare on x86 (definitely much less common
> than e.g. SIGSEGV).
>
> An example backtrace attached.
>

There is a test which runs pgbench concurrently with resizing and it
fails much less frequently (1 in 20 or even lower). Quite likely the
crash you have seen is also fixed. Please let me know if the test
fails for you and if there is something that is in your test but not
that test.

>
> 7) EXEC_BACKEND, FreeBSD
>
> We clearly need to keep this working on systems without the necessary
> bits (so likely EXEC_BACKEND, FreeBSD etc.). But the builds currently
> fail in both cases, it seems.
>
> I think it's fine to not support resizing on every platform, then we'd
> never get it, but it still needs to build. It would be good to not have
> two very different code versions, one for resizing and one without it,
> though. I wonder if we can just have the "no-resize" use the same struct
> (with the segments/mapping, ...) and all that, but skipping the space
> reservation.
>

I have not thought fully through support on platforms other than
Linux. However, my current plan is to not support GUC
max_shared_buffers and the resizing function
pg_resize_shared_buffers() on platforms which do not support the
necessary bits. The code will still build and run on those platforms,
but resizing shared buffers without restart won't be possible.

>
> 8) monitoring
>
> So, let's say I start a resize of shared buffers. How will I know what
> it's currently doing, how much longer it might take, what it's waiting
> for, etc.? I think it'd be good to have progress monitoring, through
> the regular system view (e.g. pg_stat_shmem_resize_progress?).
>

When pg_resize_shared_buffers() finishes, user knows that the resizing
is finished. It usually takes a few seconds for the resizing to
finish. I am not sure whether we will need progress reporting. But
there might be cases where the resizing has to wait for something or
it may take longer for a given phase e.g. eviction. So we may require
progress reporting. Have added TODO in the code for the same.

>
> 10) what to do about stuck resize?
>
> AFAICS the resize can get stuck for various reasons, e.g. because it
> can't evict pinned buffers, possibly indefinitely. Not great, it's not
> clear to me if there's a way out (canceling the resize) after a timeout,
> or something like that? Not great to start an "online resize" only to
> get stuck with all activity blocked for indefinite amount of time, and
> get to restart anyway.
>
> Seems related to Thomas' message [2], but AFAICS the patch does not do
> anything about this yet, right? What's the plan here?
>

pg_resize_shared_buffers() is affected by timeouts like
statement_timeouts as well as query cancellation. However, current
implementation lacks graceful handling of these events. Another TODO.

>
> 11) preparatory actions?
>
> Even if it doesn't get stuck, some of the actions can take a while, like
> evicting dirty buffers before shrinking, etc. This is similar to what
> happens on restart, when the shutdown checkpoint can take a while, while
> the system is (partly) unavailable.
>
> The common mitigation is to do an explicit checkpoint right before the
> restart, to make the shutdown checkpoint cheap. Could we do something
> similar for the shrinking, e.g. flush buffers from the part to be
> removed before actually starting the resize?
>

Eviction is carried out before changes to the memory or shared buffer
metadata. If eviction fails, the resizing is rolled back. The system
continues to work with old buffer pool size. A test particularly
testing this aspect remains to be added.

>
> 12) does this affect e.g. fork() costs?
>
> I wonder if this affects the cost of fork() in some undesirable way?
> Could it make fork() more measurably more expensive?
>

A new backend needs to attach to extra shared memory segments, AFAIK
and detach those when exiting. But I don't think there's any other
extra work that a backend needs to do when starting or exiting.
Windows might be different. I haven't seen any noticeable difference
in pgbench performance which uses new connection for every
transaction. However, we haven't tried a benchmark with empty
transactions so that fork() and exit() are exercised at a higher
frequency. Another TODO.

>
> 13) resize "state" is all over the place
>
> For me, a big hurdle when reasoning about the resizing correctness is
> that there's quite a lot of distinct pieces tracking what the current
> "state" is. I mean, there's:
>
> - ShmemCtrl->NSharedBuffers
> - NBuffers
> - NBuffersOld
> - NBuffersPending
> - ... (I'm sure I missed something)
>
> There's no cohesive description how this fits together, it seems a bit
> "ad hoc". Could be correct, but I find it hard to reason about.
>

Next set of patches will consolidate the state only in two places
NBuffersPending and StrategyControl, which needs a new name. But even
in the attached patches it's consolidated in NBuffersPending,
ShmemCtrl and StrategyControl. There are instances of NBuffers which
will be replaced by variables from ShmemCtrl or StrategyControl.

>
> 14) interesting messages from the thread
>
> While reading through the thread, I noticed a couple messages that I
> think are still relevant:
>
> - I see Peter E posted some review in 2024/11 [3], but it seems his
> comments were mostly ignored. I agree with most of them.

Find detailed reply to that email at the end.

>
> - Robert mentioned a couple interesting failure scenarios in [4], not
> sure if all of this was handled. He howerver assumes pointers would
> not be stable (and that's something we should not allow, and the
> current approach works OK in this regard, I think). He also outlines
> how it'd happen in phases - this would be useful for the design README
> I think. It also reminds me the "phases" in the checksums patch.
>

stable pointers in the latest patch. The phases are explained in the
prologue of pg_resize_shared_buffers().

> - Robert asked [5] if Linux might abruptly break this, but I find that
> unlikely. We'd point out we rely on this, and they'd likely rethink.
> This would be made safer if this was specified by POSIX - taking that
> away once implemented seems way harder than for custom extensions.
> It's likely they'd not take away the feature without an alternative
> way to achieve the same effect, I think (yes, harder to maintain).
> Tom suggests [7] this is not in POSIX.
>
> - Matthias mentioned [6] similar flags on other operating systems. Could
> some of those be used to implement the same resizing?

Part of portability TODO.

>
> - Andres had an interesting comment about how overcommit interacts with
> MAP_NORESERVE. AFAIK it means we need the flag to not break overcommit
> accounting. There's also some comments about from linux-mm people [9].
>

New implementation uses MAP_NORESERVE. See my earlier response about overcommit.

> - There seem to be some issues with releasing memory backing a mapping
> with hugetlb [10]. With the fd (and truncating the file), this seems
> to release the memory, but it's linux-specific? But most of this stuff
> is specific to linux, it seems. So is this a problem? With this it
> should be working even for hugetlb ...
>

Right. mmap() + ftruncate(), instead of mmap() + mremap() allows us to
avoid going through postmaster, which makes the implementation much
simpler. And also support hugetlb. But it will be linux only.

> - It seems FreeBSD has MFD_HUGETLB [11], so maybe we could use this and
> make the hugetlb stuff work just like on Linux? Unclear. Also, I
> thought the mfd stuff is linux-specific ... or am I confused?
>

Portability TODO.

> - Andres objected to any approach without pointer stability, and I agree
> with that. If we can figure out such solution, of course.

Since we call mmap only once, the address of the mappping does not
change; it is always stable. So pointer stability is guaranteed.

>
> - Thomas asked [13] why we need to stop all the backends, instead of
> just waiting for them to acknowledge the new (smaller) NBuffers value
> and then let them continue. I also don't quite see why this should
> not work, and it'd limit the disruption when we have to wait for
> eviction of buffers pinned by paused cursors, etc.
>

Approach in the latest patches does not stop all backends. Backends
continue to work while resizing is in progress. They are synchronized
at certain points using barriers. The details of synchronization for
each subsystem and worker backend need to be worked out. We are
working on that.

>
>
> Now, some comments about the individual patches (some of this may be a
> bit redundant with the earlier points):

Since these patches have been heavily rewritten because we have
redesigned and reimplemented the resizing process, some of the
comments may not be relevant any more. I will try to answer the
underlying concerns wherever applicable.

>
>
> v5-0001-Process-config-reload-in-AIO-workers.patch
>
> 1) Hmmm, so which other workers may need such explicit handling? Do all
> other processes participate in procsignal stuff, or does anything
> need an explicit handling?
>

As Andres mentioned in [2], this is not needed. It's not part of the
latest patches.

>
> v5-0003-Introduce-pss_barrierReceivedGeneration.patch
>
> 1) Do we actually need this? Isn't it enough to just have two barriers?
> Or a barrier + condition variable, or something like that.
>
> 2) The comment talks about "coordinated way" when processing messages,
> but it's not very clear to me. It should explain what is needed and
> not possible with the current barrier code.
>
> 3) This very much reminds me what the online checksums patch needed to
> do, and we managed to do it using plain barriers. So why does this
> need this new thing? (No opinion on whether it's correct.)
>

This isn't required in the latest implementation. Removed from the
latest patchset.

>
> v5-0004-Allow-to-use-multiple-shared-memory-mappings.patch
>
> 1) "int shmem_segment" - wouldn't it be better to have a separate enum
> for this? I mean, we'll have a predefined list of segments, right?
>

+1. Andres suggested [2] to keep only two segments. So enum may be
superfluous, but may be good if we need more segments in future. TODO
for now.

> 2) typedef struct AnonymousMapping would deserve some comment

This structure no more exists. Instead we use MemoryMappingSizes to
hold the required sizes of shared memory segments.

>
> 3) ANON_MAPPINGS - Probably should be MAX_ANON_MAPPINGS? But we'll know
> how many we have, so why not to allocate exactly the right number?
> Or even just an array of structs, like in similar cases?
>

+1. Renamed as NUM_MEMORY_MAPPINGS and is used to declare and traverse
corresponding arrays.

> 4) static int next_free_segment = 0;
>
> We exactly know what segments we'll create and in which order, no? So
> why do we even bother with this next_free_segment thing? Can't we
> simply declare an array of AnonymousMapping elements, with all the
> elements, and then just walk it and calculate the sizes/pointers?

next_free_segment is removed from the latest patches as it's not needed.

>
> 5) I'm a bit confused about the segment/mapping difference. The patch
> seems to randomly mix those, or maybe I'm just confused. I mean,
> we are creating just shmem segment, and the pieces are mappings,
> right? So why do we index them by "shmem_segment"?
>
> Also, consider
>
> CreateAnonymousSegment(AnonymousMapping *mapping)
>
> so is that creating a segment or mapping? Or what's the difference?
>
> Or are we creating multiple segments, and I missed that? Or are there
> different "segment" concepts, or what?
>
> 6) There should probably be some sort of API wrapping the mappings, so
> that the various places don't need to mess with next_free_segments
> directly, etc. Perhaps PGSharedMemoryCreate() shouldn't do this, and
> should just pass size to CreateAnonymousSegment(), and that finding
> empty slot in Mappings, etc.? Not sure that'll work, but it's a bit
> error-prone if a struct is modified from multiple places like this.

Fixed this confusion in the latest patches. There are multiple
segments, each mapped to a different address space. Since we are using
mmap() + ftruncate(), the memory allocated for each segment is
controlled by a separate fds. If we use a single address space
reservation and carve multiple segments out of it, we can not resize
each segment separately. Just to clarify, we do not reserve a large
part of address space and then carve it into smaller segments.

>
> 7) We should remember which segments got to use huge pages and which
> did not. And we should make it optional for each segment. Although,
> maybe I'm just confused about the "segment" definition - if we only
> have one, that's where huge pages are applied.
>
> If we could have multiple segments for different segments (whatever
> that means), not sure what we'll report for cases when some segments
> get to use huge pages and others don't. Either because we don't want
> to use that for some segments, or because we happen to run out of
> the available huge pages.

This is an interesting idea. In the current implementation either we
use huge pages for all the segments or none of them. I think, per
segment huge page usage will be an add-on feature in the next version.
What do you think? Using separate segments for reserving separate
address spaces will make it easy to use huge pages for some and not
for others.

>
> 8) It seems PGSharedMemoryDetach got some significant changes, but the
> comment was not modified at all. I'd guess that means the comment is
> perhaps stale, or maybe there's something we should mention.
>

Done.

> 9) I doubt the Assert on GetConfigOption needs to be repeated for all
> segments (in CreateSharedMemoryAndSemaphores).
>

Done.

> 10) Why do we have the Mapping and Segments indexed in different ways?
> I mean, Mappings seem to be filled in FIFO (just grab the next free
> slot), while Segments are indexed by segment ID.
>

Fixed this confusion in the latest patches. Both are indexed by segment ID.

> 11) Actually, what's the difference between the contents of Mappings
> and Segments? Isn't that the same thing, indexed in the same way?
> Or could it be unified? Or are they conceptually different thing?
>

See explanation above.

> 12) I believe we'll have a predefined list of segments, with fixed IDs,
> so why not just have a MAX of those IDs as the capacity?
>

yes. Fixed in the latest patches.

> 13) Would it be good to have some checks on shmem_segment values? That
> it's valid with respect to defined segments, etc. An assert, maybe?
> What about some asserts on the Mapping/Segment elements? To check
> that the element is sensible, and that the arrays "match" (if we
> need both).
>

That's a good idea. Added Asserts to that effect.

> 14) Some of the lines got pretty long, e.g. in pg_get_shmem_allocations.
> I suggest we define some macros to make this shorter, or something
> like that.
>

Done.

> 15) I'd maybe rename ShmemSegment to PGShmemSegment, for consistency
> with PGShmemHeader?

Actually we track information about the shared memory segments at
multiple places. shmem.c has APIs similar to MemoryContext for shared
memory. The information there is consolidated into ShmemSegment
structure. pg_shmem.h has platform independent APIs to manage shared
memory segments and then each implementation has implementation
specific information. Some of that information is passed from parent
process (postmaster) to child process (postgresql backends). I have
created PGInhShmemSegment for the information that is passed from
postmaster to backends and then AnonymousShmemSegment structure for
tracking information about anonymous shared memory in sysv_shmem.c. I
don't like PGInhShmemSegment name, but I haven't figured out a better
name. I may rearrange these structures a bit more in the next patches.

>
> 16) Is MAIN_SHMEM_SEGMENT something we want to expose in a public header
> file? Seems very much like an internal thing, people should access
> it only through APIs ...
>

If we convert it to an enum, we can't avoid exposing it in a public
header file. In future, we may allow users to allocate memory in
specific segments. So I think it's fine to expose it.

>
> v5-0005-Address-space-reservation-for-shared-memory.patch
>
> 1) Shouldn't reserved_offset and huge_pages_on really be in the segment
> info? Or maybe even in mapping info? (again, maybe I'm confused
> about what these structs store)

I can't find reserved_offset in the latest patches. huge_pages_on is
now a global flag, either all segments use huge pages or none of them
do. As mentioned earlier, per segment huge page usage may be an add-on
separate feature.

>
> 2) CreateSharedMemoryAndSemaphores comment is rather light on what it
> does, considering it now reserves space and then carves is into
> segments.
>

The detailed comment is in PGSharedMemoryCreate(), which seems to be a
better place for explaining memory reservation etc. I just change the
comment here to mention plural shared memory segments and
differentiate between shared memory segments and the shared memory
structures. Please let me know if this looks good.

> 3) So ReserveAnonymousMemory is what makes decisions about huge pages,
> for the whole reserved space / all segments in it. That's a bit
> unfortunate with respect to the desirability of some segments
> benefiting from huge pages and others not. Maybe we should have two
> "reserved" areas, one with huge pages, one without?
>

See my responses above about mapping vs segment and per segment huge page usage.

> I guess we don't want too many segments, because that might make
> fork() more expensive, etc. Just guessing, though. Also, how would
> this work with threading?

See my earlier response about fork() performance and also limiting
number of segments to just 2. If we do see fork() is getting
expensive, we can limit the number of segments to 1.

>
> 4) Any particular reason to define max_available_memory as
> GUC_UNIT_BLOCKS and not GUC_UNIT_MB? Of course, if we change this
> to have "max shared buffers limit" then it'd make sense to use
> blocks, but "total limit" is not in blocks.
>

Right. See response about max_shared_buffers above.

> 5) The general approach seems sound to me, but I'm not expert on this.
> I wonder how portable this behavior is. I mean, will it work on other
> Unix systems / Windows? Is it POSIX or Linux extension?

See my earlier response about portability.

>
> 6) It might be a good idea to have Assert procedures to chech mappings
> and segments (that it doesn't overflow reserved space, etc.). It
> took me ages to realize I can change shared_buffers to >60% of the
> limit, it'll happily oblige and then just crash with OOM when
> calling mprotect().
>

This shouldn't happen with the latest patches. There are tests for the
same. Please let me know if you still face the issue again in your
tests.

>
> v5-0006-Introduce-multiple-shmem-segments-for-shared-buff.patch
>
> 1) I suspect the SHMEM_RESIZE_RATIO is the wrong direction, because it
> entirely ignores relationships between the parts. See the earlier
> comment about this.
>
> 2) In fact, what happens if the user tries to resize to a value that is
> too large for one of the segments? How would the system know before
> starting the resize (and failing)?
>

+1. No SHMEM_RESIZE_RATIO in the latest patches. Reservation is
entirely based on max_shared_buffers.

> 3) It seems wrong to modify the BufferManagerShmemSize like this. It's
> probably better to have a "...SegmentSize" function for individual
> segments, and let BufferManagerShmemSize() to still return a sum of
> all segments.
>

I have reimplemented CalculateShmemSize() in the latest patches. Please review.

> 4) I think MaxAvailableMemory is the wrong abstraction, because that's
> not what people specify. See earlier comment.
>

+1. No MaxAvailableMemory in the latest patches.

> 5) Let's say we change the shared memory size (ALTER SYSTEM), trigger
> the config reload (pg_reload_conf). But then we find that we can't
> actually shrink the buffers, for some unpredictable reason (e.g.
> there's pinned buffers). How do we "undo" the change? We can't
> really undo the ALTER SYSTEM, that's already written in the .conf
> and we don't know the old value, IIRC. Is it reasonable to start
> killing backends from the assign_hook or something? Seems weird.
>

The buffer pool resizing is done using the function
pg_resize_shared_buffers(), which does not need rolling back the
config value. As mentioned earlier, appropriate error handling is
still a TBD.

>
> v5-0007-Allow-to-resize-shared-memory-without-restart.patch
>
> 1) Why would AdjustShmemSize be needed? Isn't that a sign of a bug
> somewhere in the resizing?
>

It has been replaced by BufferManagerShmemResize() in the latest
patches. BufferManagerShmemResize() only resizes the buffer manager
related shared memory segments and data structures.

> 2) Isn't the pg_memory_barrier() in CoordinateShmemResize a bit weird?
> Why is it needed, exactly? If it's to flush stuff for processes
> consuming EmitProcSignalBarrier, it's that too late? What if a
> process consumes the barrier between the emit and memory barrier?
>
> 3) WaitOnShmemBarrier seem a bit under-documented.
>

Both of these functions are not required in the new implementation.
Removed from the latest patches.

> 4) Is this actually adding buffers to the freelist? I see buf_init only
> links the new buffers by seeting freeNext, but where are the new
> buffers added to the existing freelist?
>

Freelist does not exist anymore.

> 5) The issue with a new backend seeing an old NBuffers value reminds me
> of the "support enabling checksums online" thread, where we ran into
> similar race conditions. See message [1], the part about race #2
> (the other race might be relevant too, not sure). It's been a while,
> but I think our conclusion ini that thread was that the "best" fix
> would be to change the order of steps in InitPostgres(), i.e. setup
> the ProcSignal stuff first, and only then "copy" the NBuffers value.
> And handle the possibility that we receive a "duplicate" barriers.
>

I plan to remove NBuffers entirely and instead use shared memory
variables so that we don't have to worry about backends inheriting
stale values from Postmaster or even involving Postmaster in the
resizing process. This is being worked upon and will be available in a
future patchset.

> 6) In fact, the online checksums thread seems like a possible source of
> inspiration for some of the issues, because it needs to do similar
> stuff (e.g. make sure all backends follow steps in a synchronized
> way, etc.). And it didn't need new types of Barrier to do that.
>

Right. New implementation does not require any changes to the barrier
implementation.

> 7) Also, this seems like a perfect match for testing using injection
> points. In fact, there's not a single test in the whole patch series.
> Or a single line of .sgml docs, for that matter. It took me a while
> to realize I'm supposed to change the size by ALTER SYSTEM + reload
> the config.
>

There are some tests in the latest patches. More tests are being added.

>
> v5-0008-Support-shrinking-shared-buffers.patch
>
> 1) Why is ShmemCtrl->evictor_pid reset in AnonymousShmemResize? Isn't
> there a place starting it and waiting for it to complete? Why
> shouldn't it do EvictExtraBuffers itself?
>

evictor_pid is not used in the latest patches. Eviction is done in
pg_resize_shared_buffers() itself by the backend which executes that
function.

> 2) Isn't the change to BufferManagerShmemInit wrong? How do we know the
> last buffer is still at the end of the freelist? Seems unlikely.
>

No freelist anymore.

> 3) Seems a bit strange to do it from a random backend. Shouldn't it
> be the responsibility of a process like checkpointer/bgwriter, or
> maybe a dedicated dynamic bgworker? Can we even rely on a backend
> to be available?
>

The backend which executes pg_resize_shared_buffers() coordinates the
resizing itself. I have not seen a need for it to use another
background worker yet.

> 4) Unsolved issues with buffers pinned for a long time. Could be an
> issue if the buffer is pinned indefinitely (e.g. cursor in idle
> connection), and the resizing blocks some activity (new connections
> or stuff like that).
>

Resizing stops immediately after it encounters a pinned buffer. More
sophisticated handling of such situations is a TBD and mostly v2.

> 5) Funny that "AI suggests" something, but doesn't the block fail to
> reset nextVictimBuffer of the clocksweep? It may point to a buffer
> we're removing, and it'll be invalid, no?
>

TODO:

> 6) It's not clear to me in what situations this triggers (in the call
> to BufferManagerShmemInit)
>
> if (FirstBufferToInit < NBuffers) ...
>

That code does not exist in the latest patches.

>
> v5-0009-Reinitialize-StrategyControl-after-resizing-buffe.patch
>
> 1) IMHO this should be included in the earlier resize/shrink patches,
> I don't see a reason to keep it separate (assuming this is the
> correct way, and the "init" is not).
>

Right. Merged into the resizing patch in the latest patches.

> 2) Doesn't StrategyPurgeFreeList already do some of this for the case
> of shrinking memory?

No freelist anymore.

>
> 3) Not great adding a bunch of static variables to bufmgr.c. Why do we
> need to make "everything" static global? Isn't it enough to make
> only the "valid" flag global? The rest can stay local, no?
>
> If everything needs to be global for some reason, could we at least
> make it a struct, to group the fields, not just separate random
> variables? And maybe at the top, not half-way throught the file?
>
> 4) Isn't the name BgBufferSyncAdjust misleading? It's not adjusting
> anything, it's just invalidating the info about past runs.

Being worked upon.

>
> 5) I don't quite understand why BufferSync needs to do the dance with
> delay_shmem_resize. I mean, we certainly should not run BufferSync
> from the code that resizes buffers, right? Certainly not after the
> eviction, from the part that actually rebuilds shmem structs etc.
> So perhaps something could trigger resize while we're running the
> BufferSync()? Isn't that a bit strange? If this flag is needed, it
> seems more like a band-aid for some issue in the architecture.
>
> 6) Also, why should it be fine to get into situation that some of the
> buffers might not be valid, during shrinking? I mean, why should
> this check (pg_atomic_read_u32(&ShmemCtrl->NSharedBuffers) != NBuffers).
> It seems better to ensure we never get into "sync" in a way that
> might lead some of the buffers invalid. Seems way too lowlevel to
> care about whether resize is happening.
>
> 7) I don't understand the new condition for "Execute the LRU scan".
> Won't this stop LRU scan even in cases when we want it to happen?
> Don't we want to scan the buffers in the remaining part (after
> shrinking), for example? Also, we already checked this shmem flag at
> the beginning of the function - sure, it could change (if some other
> process modifies it), but does that make sense? Wouldn't it cause
> problems if it can change at an arbitrary point while running the
> BufferSync? IMHO just another sign it may not make sense to allow
> this, i.e. buffer sync should not run during the "actual" resize.
>

Resizing runs for a few seconds. With default BufferSync frequency of
200ms, there will be a few cycles of BufferSync during resizing. If we
don't let BufferSync run during resizing, we might have more dirty
buffers to tackle post-resizing. I think it will be wise to let it run
during resizing in the portion of the buffer pool which is not being
removed. Working on that.

>
> v5-0010-Additional-validation-for-buffer-in-the-ring.patch
>
> 1) So the problem is we might create a ring before shrinking shared
> buffers, and then GetBufferFromRing will see bogus buffers? OK, but
> we should be more careful with these checks, otherwise we'll miss
> real issues when we incorrectly get an invalid buffer. Can't the
> backends do this only when they for sure know we did shrink the
> shared buffers? Or maybe even handle that during the barrier?
>

AFAIK, the buffer rings are inside the Scan nodes, which are not
accessible globally or to the barrier handling functions. So we can't
purge the rings only once during or after resizing. If we already have
a mechanism to access rings globally and hence in the barrier code, or
we can develop such a mechanism, we can purge the rings only once.
Please let me know if you have any ideas on this.

> 2) IMHO a sign there's the "transitions" between different NBuffers
> values may not be clear enough, and we're allowing stuff to happen
> in the "blurry" area. I think that's likely to cause bugs (it did
> cause issues for the online checksums patch, I think).
>

NBuffers is going to be replaced by shared memory variables. The
current code relies on buffer pool size being static for the entire
duration of the server. We are now changing that assumption. So, yes
there will be some "blurry" areas and bugs to tackle. We are
developing tests to uncover bugs and fix them.

Here's reply to email by Peter E [3].

On Tue, Nov 19, 2024 at 6:27 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>
> On 18.10.24 21:21, Dmitry Dolgov wrote:
> > v1-0001-Allow-to-use-multiple-shared-memory-mappings.patch
> >
> > Preparation, introduces the possibility to work with many shmem mappings. To
> > make it less invasive, I've duplicated the shmem API to extend it with the
> > shmem_slot argument, while redirecting the original API to it. There are
> > probably better ways of doing that, I'm open for suggestions.
>
> After studying this a bit, I tend to think you should just change the
> existing APIs in place. So for example,
>
> void *ShmemAlloc(Size size);
>
> becomes
>
> void *ShmemAlloc(int shmem_slot, Size size);
>
> There aren't that many callers, and all these duplicated interfaces
> almost add more new code than they save.
>
> It might be worth making exceptions for interfaces that are likely to be
> used by extensions. For example, I see pg_stat_statements using
> ShmemInitStruct() and ShmemInitHash(). But that seems to be it. Are
> there any other examples out there? Maybe there are many more that I
> don't see right now. But at least for the initialization functions, it
> doesn't seem worth it to preserve the existing interfaces exactly.
>
> In any case, I think the slot number should be the first argument. This
> matches how MemoryContextAlloc() or also talloc() work.
>

Fixed. Please take a look at shmem.c changes.

> (Now here is an idea: Could these just be memory contexts? Instead of
> making six shared memory slots, could you make six memory contexts with
> a special shared memory type. And ShmemAlloc becomes the allocation
> function, etc.?)

I don't see a need to do that right now. But will revisit this idea.

>
> I noticed the existing code made inconsistent use of PGShmemHeader * vs.
> void *, which also bled into your patch. I made the attached little
> patch to clean that up a bit.
>

The latest patches are rebased on top of your commit.

> I suggest splitting the struct ShmemSegment into one struct for the
> three memory addresses and a separate array just for the slock_t's. The
> former struct can then stay private in storage/ipc/shmem.c, only the
> locks need to be exported.

> Also, maybe some of this should be declared in storage/shmem.h rather
> than in storage/pg_shmem.h. We have the existing ShmemLock in there, so
> it would be a bit confusing to have the per-segment locks elsewhere.
>

I have described the new structures above. Please review.

>
> Maybe rename ANON_MAPPINGS to something like NUM_ANON_MAPPINGS.

Done.

>
> > v1-0003-Introduce-multiple-shmem-slots-for-shared-buffers.patch
> >
> > Splits shared_buffers into multiple slots, moving out structures that depend on
> > NBuffers into separate mappings. There are two large gaps here:
> >
> > * Shmem size calculation for those mappings is not correct yet, it includes too
> > many other things (no particular issues here, just haven't had time).
> > * It makes hardcoded assumptions about what is the upper limit for resizing,
> > which is currently low purely for experiments. Ideally there should be a new
> > configuration option to specify the total available memory, which would be a
> > base for subsequent calculations.
>
> Yes, I imagine a shared_buffers_hard_limit setting. We could maybe
> default that to the total available memory, but it would also be good to
> be able to specify it directly, for testing.
>

Latest patches introduce max_shared_buffers instead.

>
> > v1-0005-Use-anonymous-files-to-back-shared-memory-segment.patch
> >
> > Allows an anonyous file to back a shared mapping. This makes certain things
> > easier, e.g. mappings visual representation, and gives an fd for possible
> > future customizations.
>
> I think this could be a useful patch just by itself, without the rest of
> the series, because of
>
> > * By default, Linux will not add file-backed shared mappings into a
> > core dump, making it more convenient to work with them in PostgreSQL:
> > no more huge dumps to process.
>
> This could be significant operational benefit.
>
> When you say "by default", is this adjustable? Does someone actually
> want the whole shared memory in their core file? (If it's adjustable,
> is it also adjustable for anonymous mappings?)

'man core' mentions that /proc/[PID]/coredump_filter file controls the
memory segments written to the core file. The value in the file is a
bitmask of memory mapping types. The bits correspond to the flags
passed to mmap(). This file can be written to by the program through
/proc/self/coredump_filter or externally by something like echo >
/proc/PID/coredump_filter. A boot time setting becomes default for all
the programs. A child process inherits the setting from parent through
fork() and execve() does not change it. So it looks like DBAs should
be able to control what gets dumped in postgresql core dumps by using
any of those options. If we use file backed shared memory as the
patches do, DBAs may have to change their default setting, which may
not have considered filed backed shared memory. We will need to
highlight this in the release notes. I have added a TODO for the same
in the code for now. We will add this note in the commit message or
update relevant document as the patches get committable.

The man page says that bits 0, 1, 4 are set by default (anonymous
private and shared mappings and ELF headers). But on my machine I see
that the default is different. It's possible that different
installations use different configurations.

>
> I'm wondering about this change:
>
> -#define PG_MMAP_FLAGS
> (MAP_SHARED|MAP_ANONYMOUS|MAP_HASSEMAPHORE)
> +#define PG_MMAP_FLAGS (MAP_SHARED|MAP_HASSEMAPHORE)
>
> It looks like this would affect all mmap() calls, not only the one
> you're changing. But that's the only one that uses this macro! I don't
> understand why we need this; I don't see anything in the commit log
> about this ever being used for any portability. I think we should just
> get rid of it and have mmap() use the right flags directly.

This is committed as c100340729b66dc46d4f9d68a794957bf2c468d8.

>
> I see that FreeBSD has a memfd_create() function. Might be worth a try.
> Obviously, this whole thing needs a configure test for memfd_create()
> anyway.

We are using memfd_create in the latest patches. I will look into
creating a configure test for memfd_create(). I have added a TODO
comment for the same.

>
> I see that memfd_create() has a MFD_HUGETLB flag. It's not very clear
> how that interacts with the MAP_HUGETLB flag for mmap(). Do you need to
> specify both of them if you want huge pages?

We need both. I used toy program attached to [0] to verify that. Maybe
we could use similar program for configure test.

There are three important areas of puzzle that I will work on next:
1. Synchronization: The coordinator sends Proc signal barriers to all
the concurrent backends during resizing. The code which deals with the
buffer pool needs to react to these barriers and may need to adjust
its course of action. Checkpointer, background writer, new buffer
allocation, code scanning the buffer pool sequentially are a few
examples that need to react to the barrier code.

2. Graceful handling of errors and failures during resizing.

3. Portability: A config test to check whether a platform has the APIs
need to support this feature and enable the feature in the
corresponding build. On other platforms build succeeds, regression
runs but this feature is disabled.

All the TODOs that mentioned above and those not covered by the above
three points are added to the code. One of them being to reduce the
number of segments to just two (main and shared buffer blocks). I plan
to address them as the feature matures.

[0] https://www.postgresql.org/message-id/CAExHW5sVxEwQsuzkgjjJQP9-XVe0H2njEVw1HxeYFdT7u7J%2BeQ%40mail.gmail.com
[1] https://www.postgresql.org/message-id/CAExHW5sOu8%2B9h6t7jsA5jVcQ--N-LCtjkPnCw%2BrpoN0ovT6PHg%40mail.gmail.com
[2] https://www.postgresql.org/message-id/qltuzcdxapofdtb5mrd4em3bzu2qiwhp3cdwdsosmn7rhrtn4u%40yaogvphfwc4h
[3] https://www.postgresql.org/message-id/12add41a-7625-4639-a394-a5563e349322%40eisentraut.org
[4] https://www.postgresql.org/message-id/CAExHW5vTWABxuM5fbQcFkGuTLwaxuZDEE2vtx2WuMUWk6JnF4g%40mail.gmail.com
[5] https://www.postgresql.org/message-id/CAEze2WiMkmXUWg10y%2B_oGhJzXirZbYHB5bw0%3DVWte%2BYHwSBa%3DA%40mail.gmail.com

--
Best Wishes,
Ashutosh Bapat