Hi, On Fri, 7 Aug 2026 at 18:56, Ashutosh Bapat <[email protected]> wrote:
> > 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. > Thanks for the review! > > 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. > I initially tried to tie the cleanup to a memory context. Looking at it again, the CurrentMemoryContext fallback is not necessarily reset after an error, particularly when there is no transaction in progress. That does not seem reliable enough for this path. 0001 now allocates the request records and list cells in TopMemoryContext, like the options, and cleans them up from PG_FINALLY. Does this look closer to the intended pattern? > 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. > Thanks for this idea. I reworked the tests that way. They now use the existing callbacks, a size GUC, and injection points; the extra callback set and TAP file have been removed. > > 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. > I first changed 0002 to reset the handles as suggested. That protects the backend in which the error occurred, but I do not think it is sufficient for the next backend. The entries inserted before the error remain in ShmemIndex. If all requested entries were inserted and init_fn then failed, a later backend would find all of them and call attach_fn on areas that were never fully initialized. That is why 0002 also removes the entries inserted by the failed create attempt. This is limited to the create path: entries found on the attach path belong to an earlier successful initialization and must remain visible. ShmemIndexLock is still held during the cleanup, and the initial lookup established that none of these names existed before this attempt. This is still only a partial rollback. It does not restore the allocation offset, so the bytes remain consumed and are reported as anonymous shared memory. Reusing the names also means repeated failures could consume more of the after-startup reserve. I was not sure whether preventing a later backend from attaching to unfinished memory justifies that behavior for PG 19. Would you prefer this partial rollback, or only resetting the handles and documenting that the subsystem must detect incomplete initialization, leaving the full offset-and-index rollback for PG 20? > > > > 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. > I agree that it should not be needed there. My concern was the resulting diagnostic: it deadlocks in a normal build and reaches an unrelated state assertion in an assert build. 0003 checks the callback states and reports an error instead. Thoughts? Regards, Ayush
v2-0001-Clean-up-pending-shmem-requests-after-an-error.patch
Description: Binary data
v2-0002-Roll-back-unfinished-after-startup-shmem-initiali.patch
Description: Binary data
v2-0003-Reject-legacy-shmem-allocation-from-init-and-atta.patch
Description: Binary data
