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. 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). -- Best Wishes, Ashutosh Bapat
From 0daaf88fb76a1add42bb97ac1e557d0a9066a376 Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat <[email protected]> Date: Wed, 12 Aug 2026 11:48:34 +0530 Subject: [PATCH v20260812] Cleanup after failed shared memory request after startup If allocation or initialization of a requested shared memory area failed, CallShmemCallbacksAfterStartup() did not clear pending_shmem_requests List and reset shmem_request_state. If the list was allocated in a memory context which is cleared by error handler, pending_shmem_requests would point to non-existent memory. Next call to CallShmemCallbacksAfterStartup() would result in an Assertion failure or a crash. The stale state meant that the later shared memory requests are not processed immediately. Fix this by making CallShmemCallbacksAfterStartupCleanup() cleanup after the failure. Document that shared areas allocated before such an error remain allocated but uninitialized. The requesting subsystem should not use such areas. Ideally, we should remove the uninitialized areas and free their memory. But the fix is too invasive to be applied late in the PG 19 release cycle. Reported-by: Ayush Tiwari <[email protected]> Author: Ayush Tiwari <[email protected]> Author: Ashutosh Bapat <[email protected]> Discussion: https://www.postgresql.org/message-id/CAJTYsWVRRWH48=pcuao_2y4ap6m0qrmzxguffknrtdwk74l...@mail.gmail.com --- doc/src/sgml/xfunc.sgml | 10 ++ src/backend/storage/ipc/shmem.c | 133 ++++++++++-------- src/test/modules/test_shmem/Makefile | 3 + src/test/modules/test_shmem/meson.build | 3 + .../test_shmem/t/001_late_shmem_alloc.pl | 47 ++++++- src/test/modules/test_shmem/test_shmem.c | 26 +++- 6 files changed, 157 insertions(+), 65 deletions(-) diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index 2b8a11e7ad0..206a2838415 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -3742,6 +3742,16 @@ my_shmem_init(void *arg) lock (ShmemIndexLock), which prevents the race condition of two backends trying to initialize the memory area at the same time. </para> + <para> + If a <function>request_fn</function> callback requests multiple areas and + allocation of one of the areas fails, any areas allocated before the error + remain allocated but neither <function>init_fn</function> nor + <function>attach_fn</function> is called. If <function>init_fn</function> + fails (after all requested areas are allocated), all the areas remain + allocated and in the state in which <function>init_fn</function> leaves + them. Subsystems must detect these cases and not use unitialized or + partially initialized shared state. + </para> </sect3> <sect3 id="xfunc-shared-addin-dynamic"> diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c index a3d56cf55dd..887012d1123 100644 --- a/src/backend/storage/ipc/shmem.c +++ b/src/backend/storage/ipc/shmem.c @@ -336,6 +336,7 @@ void ShmemRequestInternal(ShmemStructOpts *options, ShmemRequestKind kind) { ShmemRequest *request; + MemoryContext oldcontext; /* Check the options */ if (options->name == NULL) @@ -374,10 +375,12 @@ ShmemRequestInternal(ShmemStructOpts *options, ShmemRequestKind kind) } /* Request looks valid, remember it */ + oldcontext = MemoryContextSwitchTo(TopMemoryContext); request = palloc(sizeof(ShmemRequest)); request->options = options; request->kind = kind; pending_shmem_requests = lappend(pending_shmem_requests, request); + MemoryContextSwitchTo(oldcontext); } /* @@ -903,75 +906,80 @@ CallShmemCallbacksAfterStartup(const ShmemCallbacks *callbacks) Assert(shmem_request_state == SRS_DONE); shmem_request_state = SRS_REQUESTING; - /* - * Call the request callback first. The callback makes ShmemRequest*() - * calls for each shmem area, adding them to pending_shmem_requests. - */ - Assert(pending_shmem_requests == NIL); - if (callbacks->request_fn) - callbacks->request_fn(callbacks->opaque_arg); - shmem_request_state = SRS_AFTER_STARTUP_ATTACH_OR_INIT; - - if (pending_shmem_requests == NIL) + PG_TRY(); { - shmem_request_state = SRS_DONE; - return; - } - - /* - * Hold ShmemIndexLock while we allocate all the shmem entries and run all - * the initializers. - */ - LWLockAcquire(ShmemIndexLock, LW_EXCLUSIVE); - - /* - * Check if the requested shared memory areas have already been - * initialized. We assume all the areas requested by the request callback - * to form a coherent unit such that they're all already initialized or - * none. Otherwise it would be ambiguous which callback, init or attach, - * to callback afterwards. - */ - found_any = notfound_any = false; - foreach_ptr(ShmemRequest, request, pending_shmem_requests) - { - if (hash_search(ShmemIndex, request->options->name, HASH_FIND, NULL)) - found_any = true; - else - notfound_any = true; - } - if (found_any && notfound_any) - elog(ERROR, "some of the requested shmem areas have already been initialized"); + /* + * Call the request callback first. The callback makes + * ShmemRequest*() calls for each shmem area, adding them to + * pending_shmem_requests. + */ + Assert(pending_shmem_requests == NIL); + if (callbacks->request_fn) + callbacks->request_fn(callbacks->opaque_arg); + shmem_request_state = SRS_AFTER_STARTUP_ATTACH_OR_INIT; - /* - * Allocate or attach all the shmem areas requested by the request_fn - * callback. - */ - foreach_ptr(ShmemRequest, request, pending_shmem_requests) - { - if (found_any) - AttachShmemIndexEntry(request, false); - else - InitShmemIndexEntry(request); + if (pending_shmem_requests) + { + /* + * Hold ShmemIndexLock while we allocate all the shmem entries and + * run all the initializers. + */ + LWLockAcquire(ShmemIndexLock, LW_EXCLUSIVE); + + /* + * Check if the requested shared memory areas have already been + * initialized. We assume all the areas requested by the request + * callback to form a coherent unit such that they're all already + * initialized or none. Otherwise it would be ambiguous which + * callback, init or attach, to callback afterwards. + */ + found_any = notfound_any = false; + foreach_ptr(ShmemRequest, request, pending_shmem_requests) + { + if (hash_search(ShmemIndex, request->options->name, HASH_FIND, NULL)) + found_any = true; + else + notfound_any = true; + } + if (found_any && notfound_any) + elog(ERROR, "some of the requested shmem areas have already been initialized"); + + /* + * Allocate or attach all the shmem areas requested by the + * request_fn callback. + */ + foreach_ptr(ShmemRequest, request, pending_shmem_requests) + { + if (found_any) + AttachShmemIndexEntry(request, false); + else + InitShmemIndexEntry(request); + } - pfree(request->options); - } - list_free_deep(pending_shmem_requests); - pending_shmem_requests = NIL; + /* Finish by calling the appropriate subsystem-specific callback. */ + if (found_any) + { + if (callbacks->attach_fn) + callbacks->attach_fn(callbacks->opaque_arg); + } + else + { + if (callbacks->init_fn) + callbacks->init_fn(callbacks->opaque_arg); + } - /* Finish by calling the appropriate subsystem-specific callback */ - if (found_any) - { - if (callbacks->attach_fn) - callbacks->attach_fn(callbacks->opaque_arg); + LWLockRelease(ShmemIndexLock); + } } - else + PG_FINALLY(); { - if (callbacks->init_fn) - callbacks->init_fn(callbacks->opaque_arg); + foreach_ptr(ShmemRequest, request, pending_shmem_requests) + pfree(request->options); + list_free_deep(pending_shmem_requests); + pending_shmem_requests = NIL; + shmem_request_state = SRS_DONE; } - - LWLockRelease(ShmemIndexLock); - shmem_request_state = SRS_DONE; + PG_END_TRY(); } /* @@ -983,6 +991,7 @@ ShmemCallRequestCallbacks(void) ListCell *lc; Assert(shmem_request_state == SRS_INITIAL); + Assert(pending_shmem_requests == NIL); shmem_request_state = SRS_REQUESTING; foreach(lc, registered_shmem_callbacks) diff --git a/src/test/modules/test_shmem/Makefile b/src/test/modules/test_shmem/Makefile index 2407f7462fe..fed8e29c8f5 100644 --- a/src/test/modules/test_shmem/Makefile +++ b/src/test/modules/test_shmem/Makefile @@ -2,6 +2,9 @@ PGFILEDESC = "test_shmem - test code for shmem allocations" +EXTRA_INSTALL = src/test/modules/injection_points +export enable_injection_points + MODULE_big = test_shmem OBJS = \ $(WIN32RES) \ diff --git a/src/test/modules/test_shmem/meson.build b/src/test/modules/test_shmem/meson.build index fb4bf328b8f..8f98f2c4e31 100644 --- a/src/test/modules/test_shmem/meson.build +++ b/src/test/modules/test_shmem/meson.build @@ -26,6 +26,9 @@ tests += { 'sd': meson.current_source_dir(), 'bd': meson.current_build_dir(), 'tap': { + 'env': { + 'enable_injection_points': get_option('injection_points') ? 'yes' : 'no', + }, 'tests': [ 't/001_late_shmem_alloc.pl', ], diff --git a/src/test/modules/test_shmem/t/001_late_shmem_alloc.pl b/src/test/modules/test_shmem/t/001_late_shmem_alloc.pl index 5cf07d071ec..7180f9deb28 100644 --- a/src/test/modules/test_shmem/t/001_late_shmem_alloc.pl +++ b/src/test/modules/test_shmem/t/001_late_shmem_alloc.pl @@ -15,8 +15,50 @@ my $node = PostgreSQL::Test::Cluster->new('main'); $node->init; $node->start; +# Test a failure in initialization of the shared memory area. +SKIP: +{ + skip "injection points not supported by this build", + if $ENV{enable_injection_points} ne 'yes'; + $node->safe_psql("postgres", "CREATE EXTENSION injection_points;"); + $node->safe_psql("postgres", + "SELECT injection_points_attach('test-shmem-init', 'error');"); + + # A failure in the requesting shared memory should not affect server + # availability. The session should remain useful, however trying to create + # the same extension again will fail. + my (undef, undef, $stderr) = + $node->psql("postgres", "CREATE EXTENSION test_shmem;"); + like($stderr, + qr/error triggered for injection point test-shmem-init/, + "failure in initialization is reported"); + $node->safe_psql("postgres", + "SELECT injection_points_detach('test-shmem-init');"); + (undef, undef, $stderr) = + $node->psql("postgres", "CREATE EXTENSION test_shmem;"); + like($stderr, qr/shmem area not yet initialized/, + "post-init extension creation fails"); + + # Only a server restart can remove the partially allocated shared memory + # area. + $node->restart; +} + +# Test failure when the request is larger than the memory reserved for +# after-startup requests. +my $session = $node->background_psql('postgres', on_error_stop => 0); +$session->query(q[SET test_shmem.area_size = '128kB';], verbose => 0); +$session->query("CREATE EXTENSION test_shmem;", verbose => 0); +like($session->{stderr}, qr/not enough shared memory/, + "an after-startup request larger than the reserve fails"); -$node->safe_psql("postgres", "CREATE EXTENSION test_shmem;"); +# The server and the backend should still be available. Since there was only one +# area requested, the failure did not change anything in the shared memory. +# Verify that the request for smaller area succeeds in the same session. +$session->{stderr} = ''; +$session->query("SET test_shmem.area_size = default;", verbose => 0); +$session->query_safe("CREATE EXTENSION test_shmem;", verbose => 0); +$session->quit; # Check that the attach counter is incremented on a new connection my $attach_count1 = @@ -28,8 +70,9 @@ cmp_ok($attach_count2, '>', $attach_count1, $node->stop; ### -# Test that loading via shared_preload_libraries also works +# Test that loading via shared_preload_libraries also works, even for large request. ### +$node->append_conf('postgresql.conf', "test_shmem.area_size = '128kB'"); $node->append_conf('postgresql.conf', "shared_preload_libraries = 'test_shmem'"); $node->start; diff --git a/src/test/modules/test_shmem/test_shmem.c b/src/test/modules/test_shmem/test_shmem.c index 9bd4012b435..2956f488135 100644 --- a/src/test/modules/test_shmem/test_shmem.c +++ b/src/test/modules/test_shmem/test_shmem.c @@ -17,9 +17,13 @@ #include "postgres.h" +#include <limits.h> + #include "fmgr.h" #include "miscadmin.h" #include "storage/shmem.h" +#include "utils/guc.h" +#include "utils/injection_point.h" PG_MODULE_MAGIC; @@ -29,11 +33,14 @@ typedef struct TestShmemData int value; bool initialized; int attach_count; + char variable_sized_array[FLEXIBLE_ARRAY_MEMBER]; } TestShmemData; static TestShmemData *TestShmem; static bool attached_or_initialized = false; +static int test_shmem_area_size = sizeof(TestShmemData); +static bool test_shmem_guc_defined = false; static void test_shmem_request(void *arg); static void test_shmem_init(void *arg); @@ -52,7 +59,7 @@ test_shmem_request(void *arg) elog(LOG, "test_shmem_request callback called"); ShmemRequestStruct(.name = "test_shmem area", - .size = sizeof(TestShmemData), + .size = test_shmem_area_size, .ptr = (void **) &TestShmem); } @@ -60,6 +67,8 @@ static void test_shmem_init(void *arg) { elog(LOG, "init callback called"); + /* Induce an error while initializing shared structure. */ + INJECTION_POINT("test-shmem-init", NULL); if (TestShmem->initialized) elog(ERROR, "shmem area already initialized"); TestShmem->initialized = true; @@ -86,6 +95,21 @@ void _PG_init(void) { elog(LOG, "test_shmem module's _PG_init called"); + + if (!test_shmem_guc_defined) + { + DefineCustomIntVariable("test_shmem.area_size", + "Size of the shmem area to request.", + NULL, + &test_shmem_area_size, + sizeof(TestShmemData), + sizeof(TestShmemData), INT_MAX, + PGC_USERSET, + GUC_UNIT_BYTE, + NULL, NULL, NULL); + MarkGUCPrefixReserved("test_shmem"); + test_shmem_guc_defined = true; + } RegisterShmemCallbacks(&TestShmemCallbacks); } base-commit: 6828b03b062bf9eee7b5e5e7ee1cedd4e174ab29 -- 2.34.1
