From d14f2939384565ad9e5d414754fa274f3f37ad4f Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat 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 CallShmemCallbacksAfterStartup() 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 Author: Ayush Tiwari Author: Ashutosh Bapat Discussion: https://www.postgresql.org/message-id/CAJTYsWVRRWH48=PcuAo_2Y4Ap6M0QRmzxgUfFkNRtdWK74LjBQ@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..1334cc55c53 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. + + If a request_fn callback requests multiple areas and + allocation of one of the areas fails, any areas allocated before the error + remain allocated but neither init_fn nor + attach_fn is called. If init_fn + fails (after all requested areas are allocated), all the areas remain + allocated and in the state in which init_fn leaves + them. Subsystems must detect these cases and not use uninitialized or + partially initialized shared state. + 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 + #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: d6bf0ab1704c62f94875d25dd71c4baa1feac5be -- 2.34.1