Hi, I have been experimenting with the new shmem registration mechanism (283e823f9dc), in particular the SHMEM_CALLBACKS_ALLOW_AFTER_STARTUP path, and ran into a few behaviours I did not expect. I may be misreading the intent here, so I would appreciate a sanity check before taking any of this further. All three reproduce on master and on REL_19_STABLE.
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.
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.
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?
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?
I have attached three small patches, one per point above, each with a
test in test_shmem. They are meant as a starting point for the
discussion rather than as finished proposals, and I am happy to rework
or drop any of them if the above points are intended decisions.
Regards,
Ayush
0001-Fix-stale-state-after-a-failed-after-startup-shmem-r.patch
Description: Binary data
0002-Preflight-after-startup-shmem-batches-for-available-.patch
Description: Binary data
0003-Reject-legacy-shmem-allocation-from-shmem-callbacks.patch
Description: Binary data
