Re: Error handling in after-startup shmem requests

On Mon, Aug 10, 2026 at 6:10 PM Ayush Tiwari
<ayushtiwari(dot)slg01(at)gmail(dot)com> wrote:
>
> 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?

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