Re: Error handling in after-startup shmem requests

Hi,

Thanks for the review!

On Mon, 10 Aug 2026 at 15:11, Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>
wrote:

> On Sun, Aug 9, 2026 at 10:00 PM Ayush Tiwari
> <ayushtiwari(dot)slg01(at)gmail(dot)com> wrote:
>
> /* Request looks valid, remember it */
> + /* Keep the requests and list cells alive until we explicitly free them.
> */
>
> The comment actually doesn't make much sense. What does it have to do
> with keeping the requests alive until freeing them with allocating
> memory in TopMemoryContext? If it has to, it should rather explain why
> we want them to be alive or why to save them in TopMemoryContext. The
> previous comment which you removed was making the point that we save
> the request "after validating" it; I would leave the wording in tact.
>
> + oldcontext = MemoryContextSwitchTo(TopMemoryContext);
>
> We should allocate the requests in the same context as the options
> itself and blow up the whole context and set the list NIL.
>

I have changed it along those lines. A request phase now creates one
memory context under TopMemoryContext. The copied options, ShmemRequest
records and list cells all live in that context, and the context is
removed when the request phase finishes.

I also changed ShmemRequestInternal() to take the size of the options
structure. This lets it validate the request first and then copy either
ShmemStructOpts, ShmemHashOpts or SlruOpts directly into the request
context. Does that seem like a reasonable way to keep the context owned
by shmem.c without exposing it to the hash and SLRU code?

>
> + PG_TRY();
> + {
> + CallShmemCallbacksAfterStartupInternal(callbacks);
>
> If you do what I suggest earlier DiscardPendingShmemRequests() simply
> becomes two statement - blow up the context, set the list NIL and rest
> shmem_request_state. I don't think we need a separate function for
> that. There is merit in having code of PG_TRY() and PG_FINALLY()
> blocks in the same function, so that it is clear what is being done in
> the try is visible when reading finally blocks. I would get rid of
> CallShmemCallbacksAfterStartupInternal() and just push the current
> code inside the PG_TRY() block and add PG_FINALLY() block after it.
>

Agreed. CallShmemCallbacksAfterStartup() now contains the complete body,
followed immediately by PG_FINALLY. FINALLY deletes the request context,
sets pending_shmem_requests to NIL and restores shmem_request_state. The
separate internal function and cleanup helper are gone. I had initially
added it
to make the diff smaller.

> +static int test_shmem_area_size = sizeof(TestShmemData);
> +static bool test_shmem_after_startup = false;
> +static char test_shmem_area_name[64] = "test_shmem area";
>
> static void test_shmem_request(void *arg);
> static void test_shmem_init(void *arg);
> @@ -51,9 +58,18 @@ test_shmem_request(void *arg)
> {
> elog(LOG, "test_shmem_request callback called");
>
> - ShmemRequestStruct(.name = "test_shmem area",
> - .size = sizeof(TestShmemData),
> + if (test_shmem_area_size == sizeof(TestShmemData))
> + strcpy(test_shmem_area_name, "test_shmem area");
> + else
> + snprintf(test_shmem_area_name, sizeof(test_shmem_area_name),
> + "test_shmem area %d", test_shmem_area_size);
> +
> + ShmemRequestStruct(.name = test_shmem_area_name,
> + .size = test_shmem_area_size,
> .ptr = (void **) &TestShmem);
>
> Heh. This is a clever idea to be able to create multiple shmem areas
> from the same module, however, we don't need this complexity in test C
> code. I would rather use $node->start, test, DROP EXTENSION,
> $node->restart, CREATE EXTENSION sequence for loading the module
> multiple times. That will also elimiate the need for the
> test_shmem_register() function and register_twice function. I would
> repeat that sequence to test the error cases first followed by
> existing tests.

> Additionally we could also test that even though the extension fails
> to load, the shared memory areas are still created when the server
> restarts if shared_preload_libraries has the extension library in it.
> But I would hesitate to add that test case if the test code becomes
> too complicated. But if you choose to add that test case, I would
> suggest the we set the GUC to just above 100K to test the failure due
> to lack of memory. Otherwise after restart the test will fail if the
> machine, where test is run, does not have 1GB memory available.
>

I simplified the tests as suggested: test_shmem now uses one fixed shmem
name, one size GUC and the existing TAP file.

I kept the two request-error attempts in the same backend, because
pending_shmem_requests and shmem_request_state are local to that backend.
A restart would clear the very state the test is meant to check. The test
therefore runs two failing CREATE EXTENSION commands in one psql session,
with an injection point just after request_fn.

For the out-of-memory case I used the restart/preload sequence you
suggested. A 128 kB request fails when test_shmem is loaded after startup,
but succeeds in a fresh cluster when test_shmem is preloaded.

> +
> + if (test_shmem_after_startup)
> + INJECTION_POINT("test-shmem-request", NULL);
> }
>
> static void
> @@ -86,7 +102,27 @@ void
> _PG_init(void)
> {
> elog(LOG, "test_shmem module's _PG_init called");
> +
> + DefineCustomIntVariable("test_shmem.area_size",
> + "Size of the shmem area to request.",
> + NULL,
> + &test_shmem_area_size,
> + sizeof(TestShmemData),
> + sizeof(TestShmemData), INT_MAX,
> + PGC_USERSET,
>
> Shouldn't this be PGC_POSTMASTER? Changing this at run time won't be
> possible.
>

I tried changing this to PGC_POSTMASTER. When test_shmem was first loaded
by CREATE EXTENSION after startup, _PG_init() failed with:

FATAL: cannot create PGC_POSTMASTER variables after startup

Pre-setting test_shmem.area_size as a placeholder gave the same result;
init_custom_variable() performs this check before placeholder replacement.
I have therefore kept it PGC_USERSET as a test-only control for the size
passed by request_fn.

> >>
> >> > 2) A request batch that only partly fits
> >> >
> >> > If a request asks for several areas and a later one does not fit, the
> >> > earlier ones stay allocated and registered. A retry then trips
> >> >
> >> > "some of the requested shmem areas have already been initialized"
> >> >
> >> > and, since shared memory is never freed, that looks permanent until a
> >> > restart. Before this mechanism each area went through its own
> >> > ShmemInitStruct() call, so the same situation could simply be retried.
> >> > Should a batch be all-or-nothing here, or is this considered
> acceptable
> >> > given that after-startup allocation is best-effort anyway?
> >>
> >> Even with ShmemInitStruct() a retry will still fail because of not
> >> enough memory. Shared memory for after startup allocation is limited,
> >> so even restarting the server won't fix it. The request has to be
> >> reduced.
> >>
> >> Did we allow calling ShmemInitStruct() at run time before this
> >> mechanism? Even if it were, the caller didn't have much choice about
> >> the areas already created. In fact the situation would be bad, since
> >> the areas which are allocated are not initialized but variables
> >> pointing them are set. So if the caller is not careful, its code may
> >> start using these areas.
> >>
> >> But with this mechanism we have choice. I think we should be able to
> >> implement all-or-nothing. At the beginning of
> >> CallShmemCallbacksAfterStartup() Remember the current allocation
> >> offset. When allocating memory remember the requests that succeeded.
> >> In case of failure, reset the allocation offset to the saved value and
> >> remove the requested entries from ShmemIndex. But that seems a lot for
> >> PG 19 at this stage. Maybe PG 20 material.
> >>
> >> I think we should document that if RegisterShmemCallbacks() called
> >> after startup throws an error, the subsystems should make sure that
> >> the shared structures are not accessed since they are not initialized,
> >> possibly setting the corresponding pointers to NULL. Or actually we
> >> should reset the pointers to NULL in CallShmemCallbacksAfterStartup()
> >> before throwing an error. That won't be invasive fix.
> >
> >
> > I first changed 0002 to reset the handles as suggested. That protects
> the
> > backend in which the error occurred, but I do not think it is sufficient
> > for the next backend. The entries inserted before the error remain in
> > ShmemIndex. If all requested entries were inserted and init_fn then
> > failed, a later backend would find all of them and call attach_fn on
> areas
> > that were never fully initialized.
> >
> > That is why 0002 also removes the entries inserted by the failed create
> > attempt. This is limited to the create path: entries found on the attach
> > path belong to an earlier successful initialization and must remain
> > visible. ShmemIndexLock is still held during the cleanup, and the
> initial
> > lookup established that none of these names existed before this attempt.
> >
> > This is still only a partial rollback. It does not restore the
> allocation
> > offset, so the bytes remain consumed and are reported as anonymous shared
> > memory. Reusing the names also means repeated failures could consume
> more
> > of the after-startup reserve. I was not sure whether preventing a later
> > backend from attaching to unfinished memory justifies that behavior for
> > PG 19. Would you prefer this partial rollback, or only resetting the
> > handles and documenting that the subsystem must detect incomplete
> > initialization, leaving the full offset-and-index rollback for PG 20?
> >
>
> Ah! I didn't see that the attach will still succeed if init_fn failed.
> I don't think just removing the entries from the ShmemIndex is enough
> without deallocating the corresponding memory. As you have rightly
> pointed out the memory is accounted as anonymous allocation memory and
> thus its source can not be investigated using pg_shmem_allocations.
>
> Here's another possibility - in each of the shmem index entry we
> maintain a flag to indicate whether the structure has been initialized
> or not. Once all the init_fns complete we set flags of all the entries
> that were added in that invocation of
> CallShmemCallbacksAfterStartup(). In the attach pathway, we set the
> pointers only for the entries which have their initialized flags set.
> For all the entries added at the time of startup the flag is set as
> the a failure in init_fn would result in a startup failure. For this
> solution, we have to maintain a list of entries and go over it after
> init_fn is called.
>
> If this solution also turns out to be invasive, I guess, we should
> just leave the things as is and document the behaviour. That's how it
> have had been without the new infrastructure. Let's improve things in
> PG 20 implementing proper rollback. Let's see what Heikki says.
>

The initialized flag seems safer than removing ShmemIndex entries without
restoring the allocation offset. It would, however, change ShmemIndexEnt
and all startup, initialization and attachment paths. I have therefore
dropped that patch from v3 and added only documentation for the current
behavior: an area allocated before init_fn fails remains discoverable, and
the subsystem must detect and avoid incomplete shared state.

> >>
> >> >
> >> > 3) Legacy allocation from an init or attach callback
> >> >
> >> > The init and attach callbacks run with ShmemIndexLock held, and
> >> > ShmemInitStruct() takes that same lock. Calling it from a callback,
> >> > which seems like a natural thing to try when moving code over from
> >> > shmem_startup_hook, trips an unrelated-looking state assertion in an
> >> > assert-enabled build and hangs in a production build. Is it worth
> >> > rejecting this explicitly, or would a note in the docs be enough?
> >>
> >> Why would ShmemInitStruct be called from a callback? The pointer to
> >> the shared structure should have been set in the given
> >> variables/addresses.
> >
> >
> > I agree that it should not be needed there. My concern was the resulting
> > diagnostic: it deadlocks in a normal build and reaches an unrelated state
> > assertion in an assert build. 0003 checks the callback states and
> reports
> > an error instead. Thoughts?
>
> See 6f7199a1245cab986a13c7b57812255fe77679d1. The document mentions
> that ShmemIndexLock is held when initializing the shared memory areas.
> It's a known fact and probably also documented that trying to acquire
> an already held LWLock causes deadlock. That's what you probably saw
> with a normal build. The error message you have added is simply
> checking the negation of the assertion. It is expected that the
> extension authors will test their extension with Assertion enabled
> build, encounter the assert and fix it. Why do we want to carry the
> error in normal builds as well?
>

Dropped that patch too.