Hi, On Wed, 12 Aug 2026 at 13:11, Ashutosh Bapat <[email protected]> wrote:
> On Tue, Aug 11, 2026 at 11:35 PM Ayush Tiwari > <[email protected]> wrote: > > > > > > 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? > > > > You are right. Thanks for the catch. Fixed in the attached version. > > >> >> > >> > >> 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? > > You are right again. We need the same session to retry. In the > attached version, I have changed the sequence of tests so that the > first test leaves a partially initialized but allocated area behind > and demonstrates how to handle such a case. The next test fails during > request and thus can be retried in the same psql session. That should > cover the stale state issues. Let me know if something is still > missing. > > > 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? > > > > The place where the injection point was placed earlier could never > have a failure. The failure can be either when request callbacks are > called or in the init callbacks not in-between. With the current > injection point placement both the cases, failure immediately after > request and also a failure in initialization are covered. > > >> > >> > > >> > 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. > > > > Hmm. Let's leave it there then. > > > 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. > > Your points are correct. Please review the latest patch and see if it > covers all the scenarios. > Thanks for the latest patch, it looks good to me. I just have 2 minor editorial nits: - The commit message mentions CallShmemCallbacksAfterStartupCleanup(), but I think that should be CallShmemCallbacksAfterStartup(). - "unitialized" in xfunc.sgml should be "uninitialized". These can be adjusted while committing so I'm not sending an updated patch. > Also please add this thread to commitfest so that it's not forgotten > and also it gets tested by CI (especially the EXEC_BACKEND case). > Here's the CF entry I had created: https://commitfest.postgresql.org/patch/7115/ And also added it to the open list item for PG 19. Regards, Ayush
