Re: Error handling in after-startup shmem requests

Hi,

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

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

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