On Sun, Aug 9, 2026 at 10:00 PM Ayush Tiwari
<[email protected]> 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
