Error handling in after-startup shmem requests

Hi Piyush,

Thanks for your report and patches.

On Thu, Aug 6, 2026 at 6:10 PM Ayush Tiwari <ayushtiwari.slg01@gmail.com> wrote:

1) Stale request state after a failed request

If an after-startup request fails, either inside the request callback or
later while the areas are being allocated, CallShmemCallbacksAfterStartup()
returns without clearing pending_shmem_requests or shmem_request_state.
A second attempt in the same backend then hits:

TRAP: failed Assert("IsPointerList(list)"), File: "list.c", Line: 341

As far as I can tell this is because the request list lives in the
caller's memory context, which error cleanup has already released. The
stale shmem_request_state also seems to make later RegisterShmemCallbacks()
calls quietly take the "remember the callbacks for later" branch.

Your analysis looks correct.

0001 collects the requests in a context of our own instead, with a reset
callback that clears pending_shmem_requests and shmem_request_state. That
way the cleanup happens on the error path as well, without adding any
PG_TRY blocks, and the startup paths are left alone. Initially I had thought of using
the TRY/CATCH blocks, but went the other way.

The fix seems more complicated than necessary.
ShmemRequestStructWithOpts() allocates the options in
TopMemoryContext, I think we should do the same with ShmemRequests or
at least the context should be child of TopMemoryContext which
outlives any query or transaction. I also think that a simple
PG_TRY/PG_FINALLY block should be enough to release all pending
requests after an error and also to set shmem_request_state. You have
mentioned that you thought of using it but did not mention why you
discarded that approach? It will save a bunch of code.

The test could use INJECTION_POINT and avoid creating a new set of
callbacks. To induce large sized failure, I would introduce a
test_shmem GUC which to decide the size of shared memory allocation
and set it to a high value before requesting memory.

You could use PG_FALLTHROUGH to avoid fall through warnings.

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.

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.

--
Best Wishes,
Ashutosh Bapat

Hi,

On Fri, 7 Aug 2026 at 18:56, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
wrote:

On Thu, Aug 6, 2026 at 6:10 PM Ayush Tiwari <ayushtiwari.slg01@gmail.com>
wrote:

1) Stale request state after a failed request

If an after-startup request fails, either inside the request callback or
later while the areas are being allocated,

CallShmemCallbacksAfterStartup()

returns without clearing pending_shmem_requests or shmem_request_state.
A second attempt in the same backend then hits:

TRAP: failed Assert("IsPointerList(list)"), File: "list.c", Line: 341

As far as I can tell this is because the request list lives in the
caller's memory context, which error cleanup has already released. The
stale shmem_request_state also seems to make later

RegisterShmemCallbacks()

calls quietly take the "remember the callbacks for later" branch.

Your analysis looks correct.

Thanks for the review!

0001 collects the requests in a context of our own instead, with a reset
callback that clears pending_shmem_requests and shmem_request_state.

That

way the cleanup happens on the error path as well, without adding any
PG_TRY blocks, and the startup paths are left alone. Initially I had

thought of using

the TRY/CATCH blocks, but went the other way.

The fix seems more complicated than necessary.
ShmemRequestStructWithOpts() allocates the options in
TopMemoryContext, I think we should do the same with ShmemRequests or
at least the context should be child of TopMemoryContext which
outlives any query or transaction. I also think that a simple
PG_TRY/PG_FINALLY block should be enough to release all pending
requests after an error and also to set shmem_request_state. You have
mentioned that you thought of using it but did not mention why you
discarded that approach? It will save a bunch of code.

I initially tried to tie the cleanup to a memory context. Looking at it
again, the CurrentMemoryContext fallback is not necessarily reset after
an error, particularly when there is no transaction in progress. That
does not seem reliable enough for this path.

0001 now allocates the request records and list cells in
TopMemoryContext, like the options, and cleans them up from PG_FINALLY.
Does this look closer to the intended pattern?

The test could use INJECTION_POINT and avoid creating a new set of
callbacks. To induce large sized failure, I would introduce a
test_shmem GUC which to decide the size of shared memory allocation
and set it to a high value before requesting memory.

Thanks for this idea.
I reworked the tests that way. They now use the existing callbacks, a
size GUC, and injection points; the extra callback set and TAP file have
been removed.

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?

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?

Regards,
Ayush

On Sun, Aug 9, 2026 at 10:00 PM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:

0001 collects the requests in a context of our own instead, with a reset
callback that clears pending_shmem_requests and shmem_request_state. That
way the cleanup happens on the error path as well, without adding any
PG_TRY blocks, and the startup paths are left alone. Initially I had thought of using
the TRY/CATCH blocks, but went the other way.

The fix seems more complicated than necessary.
ShmemRequestStructWithOpts() allocates the options in
TopMemoryContext, I think we should do the same with ShmemRequests or
at least the context should be child of TopMemoryContext which
outlives any query or transaction. I also think that a simple
PG_TRY/PG_FINALLY block should be enough to release all pending
requests after an error and also to set shmem_request_state. You have
mentioned that you thought of using it but did not mention why you
discarded that approach? It will save a bunch of code.

I initially tried to tie the cleanup to a memory context. Looking at it
again, the CurrentMemoryContext fallback is not necessarily reset after
an error, particularly when there is no transaction in progress. That
does not seem reliable enough for this path.

0001 now allocates the request records and list cells in
TopMemoryContext, like the options, and cleans them up from PG_FINALLY.
Does this look closer to the intended pattern?

The test could use INJECTION_POINT and avoid creating a new set of
callbacks. To induce large sized failure, I would introduce a
test_shmem GUC which to decide the size of shared memory allocation
and set it to a high value before requesting memory.

Thanks for this idea.
I reworked the tests that way. They now use the existing callbacks, a
size GUC, and injection points; the extra callback set and TAP file have
been removed.

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

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

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

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

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.

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?

--
Best Wishes,
Ashutosh Bapat

Hi,

Thanks for the review!

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

On Sun, Aug 9, 2026 at 10:00 PM Ayush Tiwari
<ayushtiwari.slg01@gmail.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.

On Mon, Aug 10, 2026 at 6:10 PM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:

Hi,

Thanks for the review!

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

On Sun, Aug 9, 2026 at 10:00 PM Ayush Tiwari
<ayushtiwari.slg01@gmail.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?

What I had in mind is a child of TopMemoryContext to be used for
savings options, list and requests which will be reset instead of
deleting it after every cycle of allocations. But I don't think even
that is required. In the attached patch, I have allocated all of it in
TopMemoryContext and freed it at appropriate places including the
PG_FINALLY block. The changes look much more sensible and simple now.
Let me know what you think.

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

Node creation is an expensive operation. We should reuse it as much as
possible, like attached.

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.

Thanks for the explanation. Why do we need test_shmem_guc_defined?

I have rewritten the test to avoid creating nodes, or even restarts.
Please check if it still tests the intended scenarios.

--
Best Wishes,
Ashutosh Bapat

Hi,

On Tue, 11 Aug 2026 at 21:16, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
wrote:

On Mon, Aug 10, 2026 at 6:10 PM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:

Hi,

Thanks for the review!

On Mon, 10 Aug 2026 at 15:11, Ashutosh Bapat <

ashutosh.bapat.oss@gmail.com> wrote:

On Sun, Aug 9, 2026 at 10:00 PM Ayush Tiwari
<ayushtiwari.slg01@gmail.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?

What I had in mind is a child of TopMemoryContext to be used for
savings options, list and requests which will be reset instead of
deleting it after every cycle of allocations. But I don't think even
that is required. In the attached patch, I have allocated all of it in
TopMemoryContext and freed it at appropriate places including the
PG_FINALLY block. The changes look much more sensible and simple now.
Let me know what you think.

Thanks for the updated patch.

Yes, this does look simpler to me than adding a separate child context.

One detail caught my attention: the no-request branch now returns from
inside
PG_TRY. Could that skip PG_FINALLY/PG_END_TRY and leave the saved error
stack
unrestored? Would it be safer to guard the allocation work with
`pending_shmem_requests != NIL`, allowing every path to reach the common
cleanup instead?

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

Node creation is an expensive operation. We should reuse it as much as
possible, like attached.

Agreed on reusing the node. Since the stale list is backend-local, do
separate
`$node->psql()` calls use different backends and miss the retry path? I
tried
the test with one background psql session. With the init_fn injection, the
area was already indexed but uninitialized, and the later CREATE EXTENSION
failed in test_shmem_attach(), consistent with the new documentation. Would
an injection immediately after request_fn be closer to the original failure?

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.

Thanks for the explanation. Why do we need test_shmem_guc_defined?

It is needed for the same-backend retry. A failed CREATE EXTENSION can
leave
the library mapped, but it is not added to the successfully-loaded library
list until _PG_init() returns, so the retry invokes _PG_init() again. When
I
removed the guard, the second attempt failed with `attempt to redefine
parameter "test_shmem.area_size"` before reaching the shmem retry path.

I have rewritten the test to avoid creating nodes, or even restarts.
Please check if it still tests the intended scenarios.

With an injection immediately after request_fn and fail/fail/succeed in one
backend, your cleanup fixed the original retry failure in my testing.
Please
let me know if I have misunderstood any of the points above.

Regards,
Ayush

On Tue, Aug 11, 2026 at 11:35 PM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:

One detail caught my attention: the no-request branch now returns from inside
PG_TRY. Could that skip PG_FINALLY/PG_END_TRY and leave the saved error stack
unrestored? Would it be safer to guard the allocation work with
`pending_shmem_requests != NIL`, allowing every path to reach the common
cleanup instead?

You are right. Thanks for the catch. Fixed in the attached version.

Node creation is an expensive operation. We should reuse it as much as
possible, like attached.

Agreed on reusing the node. Since the stale list is backend-local, do separate
`$node->psql()` calls use different backends and miss the retry path?

You are right again. We need the same session to retry. In the
attached version, I have changed the sequence of tests so that the
first test leaves a partially initialized but allocated area behind
and demonstrates how to handle such a case. The next test fails during
request and thus can be retried in the same psql session. That should
cover the stale state issues. Let me know if something is still
missing.

I tried
the test with one background psql session. With the init_fn injection, the
area was already indexed but uninitialized, and the later CREATE EXTENSION
failed in test_shmem_attach(), consistent with the new documentation. Would
an injection immediately after request_fn be closer to the original failure?

The place where the injection point was placed earlier could never
have a failure. The failure can be either when request callbacks are
called or in the init callbacks not in-between. With the current
injection point placement both the cases, failure immediately after
request and also a failure in initialization are covered.

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.

Thanks for the explanation. Why do we need test_shmem_guc_defined?

It is needed for the same-backend retry. A failed CREATE EXTENSION can leave
the library mapped, but it is not added to the successfully-loaded library
list until _PG_init() returns, so the retry invokes _PG_init() again. When I
removed the guard, the second attempt failed with `attempt to redefine
parameter "test_shmem.area_size"` before reaching the shmem retry path.

Hmm. Let's leave it there then.

With an injection immediately after request_fn and fail/fail/succeed in one
backend, your cleanup fixed the original retry failure in my testing. Please
let me know if I have misunderstood any of the points above.

Your points are correct. Please review the latest patch and see if it
covers all the scenarios.

Also please add this thread to commitfest so that it's not forgotten
and also it gets tested by CI (especially the EXEC_BACKEND case).

--
Best Wishes,
Ashutosh Bapat

Hi,

On Wed, 12 Aug 2026 at 13:11, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
wrote:

On Tue, Aug 11, 2026 at 11:35 PM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:

One detail caught my attention: the no-request branch now returns from

inside

PG_TRY. Could that skip PG_FINALLY/PG_END_TRY and leave the saved error

stack

unrestored? Would it be safer to guard the allocation work with
`pending_shmem_requests != NIL`, allowing every path to reach the common
cleanup instead?

You are right. Thanks for the catch. Fixed in the attached version.

Node creation is an expensive operation. We should reuse it as much as
possible, like attached.

Agreed on reusing the node. Since the stale list is backend-local, do

separate

`$node->psql()` calls use different backends and miss the retry path?

You are right again. We need the same session to retry. In the
attached version, I have changed the sequence of tests so that the
first test leaves a partially initialized but allocated area behind
and demonstrates how to handle such a case. The next test fails during
request and thus can be retried in the same psql session. That should
cover the stale state issues. Let me know if something is still
missing.

I tried
the test with one background psql session. With the init_fn injection,

the

area was already indexed but uninitialized, and the later CREATE

EXTENSION

failed in test_shmem_attach(), consistent with the new documentation.

Would

an injection immediately after request_fn be closer to the original

failure?

The place where the injection point was placed earlier could never
have a failure. The failure can be either when request callbacks are
called or in the init callbacks not in-between. With the current
injection point placement both the cases, failure immediately after
request and also a failure in initialization are covered.

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.

Thanks for the explanation. Why do we need test_shmem_guc_defined?

It is needed for the same-backend retry. A failed CREATE EXTENSION can

leave

the library mapped, but it is not added to the successfully-loaded

library

list until _PG_init() returns, so the retry invokes _PG_init() again.

When I

removed the guard, the second attempt failed with `attempt to redefine
parameter "test_shmem.area_size"` before reaching the shmem retry path.

Hmm. Let's leave it there then.

With an injection immediately after request_fn and fail/fail/succeed in

one

backend, your cleanup fixed the original retry failure in my testing.

Please

let me know if I have misunderstood any of the points above.

Your points are correct. Please review the latest patch and see if it
covers all the scenarios.

Thanks for the latest patch, it looks good to me.

I just have 2 minor editorial nits:

- The commit message mentions CallShmemCallbacksAfterStartupCleanup(), but I
think that should be CallShmemCallbacksAfterStartup().

- "unitialized" in xfunc.sgml should be "uninitialized".

These can be adjusted while committing so I'm not sending an updated patch.

Also please add this thread to commitfest so that it's not forgotten
and also it gets tested by CI (especially the EXEC_BACKEND case).

Here's the CF entry I had created:
https://commitfest.postgresql.org/patch/7115/

And also added it to the open list item for PG 19.

Regards,
Ayush

On Wed, Aug 12, 2026 at 2:37 PM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:

Thanks for the latest patch, it looks good to me.

Thanks.

I just have 2 minor editorial nits:

- The commit message mentions CallShmemCallbacksAfterStartupCleanup(), but I
think that should be CallShmemCallbacksAfterStartup().

- "unitialized" in xfunc.sgml should be "uninitialized".

These can be adjusted while committing so I'm not sending an updated patch.

Attached patch fixes those two as well.

Here's the CF entry I had created:
https://commitfest.postgresql.org/patch/7115/

I see Meson MacOS build failing, but the issue seems to be with
downloading dependencies. The CI will be triggered again with a new
patch and hopefully we will see the failure gone.

--
Best Wishes,
Ashutosh Bapat