From 6c42a8210c742914ef72d572b24351521a1017d7 Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat Date: Tue, 11 Aug 2026 20:51:35 +0530 Subject: [PATCH v20260811] 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 Author: Ayush Tiwari Author: Ashutosh Bapat --- doc/src/sgml/xfunc.sgml | 8 ++ src/backend/storage/ipc/shmem.c | 131 ++++++++++-------- src/test/modules/test_shmem/Makefile | 3 + src/test/modules/test_shmem/meson.build | 3 + .../test_shmem/t/001_late_shmem_alloc.pl | 35 ++++- src/test/modules/test_shmem/test_shmem.c | 26 +++- 6 files changed, 142 insertions(+), 64 deletions(-) diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index 2b8a11e7ad0..af776bdbfa3 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -3742,6 +3742,14 @@ 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 not initialized by init_fn. If + init_fn fails, all the areas remain allocated and in + the state in which init_fn leaves them. Subsystems + must detect that case and not use incompletely initialized shared state. + diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c index a3d56cf55dd..477635829f0 100644 --- a/src/backend/storage/ipc/shmem.c +++ b/src/backend/storage/ipc/shmem.c @@ -142,6 +142,7 @@ #include "storage/shmem_internal.h" #include "storage/spin.h" #include "utils/builtins.h" +#include "utils/memutils.h" #include "utils/tuplestore.h" /* @@ -336,6 +337,7 @@ void ShmemRequestInternal(ShmemStructOpts *options, ShmemRequestKind kind) { ShmemRequest *request; + MemoryContext oldcontext; /* Check the options */ if (options->name == NULL) @@ -374,10 +376,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 +907,83 @@ 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; - } + /* + * 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; - /* - * Hold ShmemIndexLock while we allocate all the shmem entries and run all - * the initializers. - */ - LWLockAcquire(ShmemIndexLock, LW_EXCLUSIVE); + if (pending_shmem_requests == NIL) + { + shmem_request_state = SRS_DONE; + return; + } - /* - * 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"); + /* + * Hold ShmemIndexLock while we allocate all the shmem entries and run + * all the initializers. + */ + LWLockAcquire(ShmemIndexLock, LW_EXCLUSIVE); - /* - * Allocate or attach all the shmem areas requested by the request_fn - * callback. - */ - foreach_ptr(ShmemRequest, request, pending_shmem_requests) - { + /* + * 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); + } + + /* Finish by calling the appropriate subsystem-specific callback. */ if (found_any) - AttachShmemIndexEntry(request, false); + { + if (callbacks->attach_fn) + callbacks->attach_fn(callbacks->opaque_arg); + } else - InitShmemIndexEntry(request); - - pfree(request->options); - } - list_free_deep(pending_shmem_requests); - pending_shmem_requests = NIL; + { + 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 +995,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..ede26a067aa 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 @@ -13,10 +13,37 @@ use Test::More; ### my $node = PostgreSQL::Test::Cluster->new('main'); $node->init; + +# Test failure when the request is larger than the memory reserved for +# after-startup requests. $node->start; +my (undef, undef, $oom_stderr) = $node->psql("postgres", q[ +SET test_shmem.area_size = '128kB'; +CREATE EXTENSION test_shmem;]); +like($oom_stderr, qr/not enough shared memory/, + "an after-startup request larger than the reserve fails"); +# A failure in the requesting shared memory should not affect server +# availability. We should still be able to try to create the extension again. +# Test a failure in initialization of the shared memory area. +SKIP: +{ + skip "injection points not supported by this build", 2 + 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');"); + ok($node->psql("postgres", "CREATE EXTENSION test_shmem;"), + "request callback failure is reported on both attempts"); + $node->safe_psql("postgres", + "SELECT injection_points_detach('test-shmem-init');"); +} -$node->safe_psql("postgres", "CREATE EXTENSION test_shmem;"); +# The server should still be available and verify that the request succeeds for +# smaller request. +$node->safe_psql("postgres", q[ +SET test_shmem.area_size = default; +CREATE EXTENSION test_shmem;]); # Check that the attach counter is incremented on a new connection my $attach_count1 = @@ -28,10 +55,10 @@ 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', - "shared_preload_libraries = 'test_shmem'"); +$node->append_conf('postgresql.conf', "test_shmem.area_size = '128kB'"); +$node->append_conf('postgresql.conf', "shared_preload_libraries = 'test_shmem'"); $node->start; # When loaded via shared_preload_libraries, the attach callback is 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: 1d1d7b0e9c99a6c7b2bb874b5c7f6806e5940f04 -- 2.34.1