Hi Piyush, Thanks for your report and patches.
On Thu, Aug 6, 2026 at 6:10 PM Ayush Tiwari <[email protected]> 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
