From ba55c513a80b3eae391359893f3bbdeb055e4ba6 Mon Sep 17 00:00:00 2001 From: Ayush Tiwari Date: Mon, 10 Aug 2026 17:10:25 +0530 Subject: [PATCH v3] Fix cleanup after failed after-startup shmem requests CallShmemCallbacksAfterStartup() cleared pending_shmem_requests and reset shmem_request_state only on success. The request records and list cells lived in the caller's memory context while the options lived in TopMemoryContext, so an error could leave the global list pointing at released memory. A second attempt in the same backend then walked a dangling list and failed the IsPointerList assertion. The stale state also made later registrations take the "remember for later" path instead of running immediately. Allocate the options, request records and list cells in one child of TopMemoryContext. Wrap the complete after-startup operation in PG_TRY and PG_FINALLY, deleting that context and restoring shmem_request_state on both success and error. Use the same context for startup and EXEC_BACKEND request collection, where successful cleanup can delete it directly. Document that shared areas allocated before a later error cannot be reclaimed and must not be used unless init_fn completed. This patch does not attempt to roll back shared allocation or ShmemIndex entries. Add focused tests for errors immediately after request_fn and during late allocation. The request error is retried in one backend, because the damaged state is backend-local. Also verify that the same 128 kB request which fails when loaded late succeeds when test_shmem is preloaded. Author: Ayush Tiwari --- doc/src/sgml/xfunc.sgml | 7 + src/backend/access/transam/slru.c | 14 +- src/backend/storage/ipc/shmem.c | 185 ++++++++++-------- src/backend/storage/ipc/shmem_hash.c | 15 +- src/include/storage/shmem_internal.h | 3 +- src/test/modules/test_shmem/Makefile | 3 + src/test/modules/test_shmem/meson.build | 3 + .../test_shmem/t/001_late_shmem_alloc.pl | 45 +++++ src/test/modules/test_shmem/test_shmem.c | 22 ++- 9 files changed, 196 insertions(+), 101 deletions(-) diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml index 2b8a11e7ad0..f7d095e4640 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -3742,6 +3742,13 @@ 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 after-startup initialization fails, any areas allocated before the + error remain in the shared memory index. A later backend can therefore + find them even though init_fn did not complete. + Subsystems must detect that case and not use incompletely initialized + shared state. + diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c index 47dd52d6749..4e89d797772 100644 --- a/src/backend/access/transam/slru.c +++ b/src/backend/access/transam/slru.c @@ -245,21 +245,21 @@ SimpleLruAutotuneBuffers(int divisor, int max) void SimpleLruRequestWithOpts(const SlruOpts *options) { - SlruOpts *options_copy; + SlruOpts options_copy; Assert(options->name != NULL); Assert(options->nslots > 0); Assert(options->PagePrecedes != NULL); Assert(options->errdetail_for_io_error != NULL); - options_copy = MemoryContextAlloc(TopMemoryContext, - sizeof(SlruOpts)); - memcpy(options_copy, options, sizeof(SlruOpts)); + memcpy(&options_copy, options, sizeof(SlruOpts)); - options_copy->base.name = options->name; - options_copy->base.size = SimpleLruShmemSize(options_copy->nslots, options_copy->nlsns); + options_copy.base.name = options->name; + options_copy.base.size = SimpleLruShmemSize(options_copy.nslots, + options_copy.nlsns); - ShmemRequestInternal(&options_copy->base, SHMEM_KIND_SLRU); + ShmemRequestInternal(&options_copy.base, sizeof(SlruOpts), + SHMEM_KIND_SLRU); } /* Initialize locks and shared memory area */ diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c index a3d56cf55dd..0a04b0e83c0 100644 --- a/src/backend/storage/ipc/shmem.c +++ b/src/backend/storage/ipc/shmem.c @@ -142,6 +142,8 @@ #include "storage/shmem_internal.h" #include "storage/spin.h" #include "utils/builtins.h" +#include "utils/injection_point.h" +#include "utils/memutils.h" #include "utils/tuplestore.h" /* @@ -167,6 +169,7 @@ typedef struct } ShmemRequest; static List *pending_shmem_requests; +static MemoryContext pending_shmem_requests_context; /* * Per-process state machine, for sanity checking that we do things in the @@ -314,28 +317,23 @@ Datum pg_numa_available(PG_FUNCTION_ARGS); void ShmemRequestStructWithOpts(const ShmemStructOpts *options) { - ShmemStructOpts *options_copy; - - options_copy = MemoryContextAlloc(TopMemoryContext, - sizeof(ShmemStructOpts)); - memcpy(options_copy, options, sizeof(ShmemStructOpts)); - - ShmemRequestInternal(options_copy, SHMEM_KIND_STRUCT); + ShmemRequestInternal(options, sizeof(ShmemStructOpts), SHMEM_KIND_STRUCT); } /* * Internal workhorse of ShmemRequestStruct() and ShmemRequestHash(). * - * Note: Unlike in the public ShmemRequestStruct() and ShmemRequestHash() - * functions, 'options' is *not* copied. It must be allocated in - * TopMemoryContext by the caller, and will be freed after the init/attach - * callbacks have been called. This allows ShmemRequestHash() to pass a - * pointer to the extended ShmemHashOpts struct instead. + * 'options_size' can be larger than ShmemStructOpts, allowing callers such as + * ShmemRequestHash() to pass an extended options structure. ShmemStructOpts + * must be its first member. */ void -ShmemRequestInternal(ShmemStructOpts *options, ShmemRequestKind kind) +ShmemRequestInternal(const ShmemStructOpts *options, Size options_size, + ShmemRequestKind kind) { ShmemRequest *request; + ShmemStructOpts *options_copy; + MemoryContext oldcontext; /* Check the options */ if (options->name == NULL) @@ -374,10 +372,14 @@ ShmemRequestInternal(ShmemStructOpts *options, ShmemRequestKind kind) } /* Request looks valid, remember it */ + oldcontext = MemoryContextSwitchTo(pending_shmem_requests_context); + options_copy = palloc(options_size); + memcpy(options_copy, options, options_size); request = palloc(sizeof(ShmemRequest)); - request->options = options; + request->options = options_copy; request->kind = kind; pending_shmem_requests = lappend(pending_shmem_requests, request); + MemoryContextSwitchTo(oldcontext); } /* @@ -433,11 +435,10 @@ ShmemInitRequested(void) * so no need for locking. */ foreach_ptr(ShmemRequest, request, pending_shmem_requests) - { InitShmemIndexEntry(request); - pfree(request->options); - } - list_free_deep(pending_shmem_requests); + + MemoryContextDelete(pending_shmem_requests_context); + pending_shmem_requests_context = NULL; pending_shmem_requests = NIL; /* @@ -476,11 +477,10 @@ ShmemAttachRequested(void) * Attach to all the requested memory areas. */ foreach_ptr(ShmemRequest, request, pending_shmem_requests) - { AttachShmemIndexEntry(request, false); - pfree(request->options); - } - list_free_deep(pending_shmem_requests); + + MemoryContextDelete(pending_shmem_requests_context); + pending_shmem_requests_context = NULL; pending_shmem_requests = NIL; /* Call attach callbacks */ @@ -744,7 +744,12 @@ ResetShmemAllocator(void) Assert(!IsUnderPostmaster); shmem_request_state = SRS_INITIAL; - pending_shmem_requests = NIL; + if (pending_shmem_requests_context != NULL) + { + MemoryContextDelete(pending_shmem_requests_context); + pending_shmem_requests_context = NULL; + pending_shmem_requests = NIL; + } /* * Note that we don't clear the registered callbacks. We will need to @@ -901,77 +906,83 @@ CallShmemCallbacksAfterStartup(const ShmemCallbacks *callbacks) bool notfound_any; 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; + Assert(pending_shmem_requests_context == NULL); + pending_shmem_requests_context = + AllocSetContextCreate(TopMemoryContext, + "Pending shmem requests", + ALLOCSET_SMALL_SIZES); + shmem_request_state = SRS_REQUESTING; - 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); + /* + * Call the request callback first. The callback makes ShmemRequest*() + * calls for each shmem area, adding them to pending_shmem_requests. + */ + if (callbacks->request_fn) + callbacks->request_fn(callbacks->opaque_arg); + INJECTION_POINT("shmem-after-startup-request", NULL); + shmem_request_state = SRS_AFTER_STARTUP_ATTACH_OR_INIT; - /* - * 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"); + if (pending_shmem_requests != NIL) + { + /* + * 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); + /* Allocate or attach all the requested shmem areas. */ + 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); + MemoryContextDelete(pending_shmem_requests_context); + pending_shmem_requests_context = NULL; + pending_shmem_requests = NIL; + shmem_request_state = SRS_DONE; } - - LWLockRelease(ShmemIndexLock); - shmem_request_state = SRS_DONE; + PG_END_TRY(); } /* @@ -983,6 +994,12 @@ ShmemCallRequestCallbacks(void) ListCell *lc; Assert(shmem_request_state == SRS_INITIAL); + Assert(pending_shmem_requests == NIL); + Assert(pending_shmem_requests_context == NULL); + pending_shmem_requests_context = + AllocSetContextCreate(TopMemoryContext, + "Pending shmem requests", + ALLOCSET_SMALL_SIZES); shmem_request_state = SRS_REQUESTING; foreach(lc, registered_shmem_callbacks) diff --git a/src/backend/storage/ipc/shmem_hash.c b/src/backend/storage/ipc/shmem_hash.c index c28d673cbd2..8d9281d6284 100644 --- a/src/backend/storage/ipc/shmem_hash.c +++ b/src/backend/storage/ipc/shmem_hash.c @@ -43,20 +43,19 @@ static void *ShmemHashAlloc(Size size, void *alloc_arg); void ShmemRequestHashWithOpts(const ShmemHashOpts *options) { - ShmemHashOpts *options_copy; + ShmemHashOpts options_copy; Assert(options->name != NULL); - options_copy = MemoryContextAlloc(TopMemoryContext, - sizeof(ShmemHashOpts)); - memcpy(options_copy, options, sizeof(ShmemHashOpts)); + memcpy(&options_copy, options, sizeof(ShmemHashOpts)); /* Set options for the fixed-size area holding the hash table */ - options_copy->base.name = options->name; - options_copy->base.size = hash_estimate_size(options_copy->nelems, - options_copy->hash_info.entrysize); + options_copy.base.name = options->name; + options_copy.base.size = hash_estimate_size(options_copy.nelems, + options_copy.hash_info.entrysize); - ShmemRequestInternal(&options_copy->base, SHMEM_KIND_HASH); + ShmemRequestInternal(&options_copy.base, sizeof(ShmemHashOpts), + SHMEM_KIND_HASH); } void diff --git a/src/include/storage/shmem_internal.h b/src/include/storage/shmem_internal.h index 8746b614fa3..9d337505986 100644 --- a/src/include/storage/shmem_internal.h +++ b/src/include/storage/shmem_internal.h @@ -34,7 +34,8 @@ extern void AttachShmemAllocator(PGShmemHeader *seghdr); #endif extern void ResetShmemAllocator(void); -extern void ShmemRequestInternal(ShmemStructOpts *options, ShmemRequestKind kind); +extern void ShmemRequestInternal(const ShmemStructOpts *options, + Size options_size, ShmemRequestKind kind); extern size_t ShmemGetRequestedSize(void); extern void ShmemInitRequested(void); 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..013abbf4524 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 @@ -16,6 +16,51 @@ $node->init; $node->start; +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('shmem-after-startup-request', 'error');"); + my $session = $node->background_psql('postgres', on_error_stop => 0); + my (undef, $failed1) = $session->query("CREATE EXTENSION test_shmem;"); + my (undef, $failed2) = $session->query("CREATE EXTENSION test_shmem;"); + $session->quit; + ok($failed1 && $failed2, + "request callback failure is reported on both attempts"); + is($node->safe_psql("postgres", "SELECT 1"), '1', + "request callback failure does not crash the backend"); + $node->safe_psql("postgres", + "SELECT injection_points_detach('shmem-after-startup-request');"); +} + +$node->stop; + +my $oom_node = PostgreSQL::Test::Cluster->new('oom'); +$oom_node->init; +$oom_node->start; +my (undef, undef, $oom_stderr) = $oom_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"); +$oom_node->stop; + +my $preload_node = PostgreSQL::Test::Cluster->new('preload_large'); +$preload_node->init; +$preload_node->append_conf('postgresql.conf', q[ +test_shmem.area_size = '128kB' +shared_preload_libraries = 'test_shmem']); +$preload_node->start; +$preload_node->safe_psql("postgres", "CREATE EXTENSION test_shmem;"); +is($preload_node->safe_psql("postgres", "SELECT get_test_shmem_attach_count();"), + '0', "the same request succeeds when test_shmem is preloaded"); +$preload_node->stop; + +$node = PostgreSQL::Test::Cluster->new('normal'); +$node->init; +$node->start; $node->safe_psql("postgres", "CREATE EXTENSION test_shmem;"); # Check that the attach counter is incremented on a new connection diff --git a/src/test/modules/test_shmem/test_shmem.c b/src/test/modules/test_shmem/test_shmem.c index 9bd4012b435..089c364798e 100644 --- a/src/test/modules/test_shmem/test_shmem.c +++ b/src/test/modules/test_shmem/test_shmem.c @@ -17,9 +17,12 @@ #include "postgres.h" +#include + #include "fmgr.h" #include "miscadmin.h" #include "storage/shmem.h" +#include "utils/guc.h" PG_MODULE_MAGIC; @@ -34,6 +37,8 @@ typedef struct 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 +57,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); } @@ -86,6 +91,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); } -- 2.34.1