Hi, On Tue, 11 Aug 2026 at 21:16, Ashutosh Bapat <[email protected]> wrote:
> On Mon, Aug 10, 2026 at 6:10 PM Ayush Tiwari > <[email protected]> wrote: > > > > Hi, > > > > Thanks for the review! > > > > On Mon, 10 Aug 2026 at 15:11, Ashutosh Bapat < > [email protected]> wrote: > >> > >> On Sun, Aug 9, 2026 at 10:00 PM Ayush Tiwari > >> <[email protected]> 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
