Re: 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(dot)slg01(at)gmail(dot)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