From aa618c279b52c796f1989bf1cb0f294570be1e0b Mon Sep 17 00:00:00 2001 From: Ayush Tiwari Date: Thu, 6 Aug 2026 13:27:29 +0530 Subject: [PATCH 1/3] Fix stale state after a failed after-startup shmem request CallShmemCallbacksAfterStartup() reset pending_shmem_requests and shmem_request_state only on success. If its request callback or a later allocation failed, it left both behind. The requests and their List were allocated in the caller's memory context, so error cleanup could free the List while pending_shmem_requests still pointed to it. A retry in the same backend then crashed: TRAP: failed Assert("IsPointerList(list)"), File: "list.c", Line: 341 The stale shmem_request_state also made later calls to RegisterShmemCallbacks() silently take the startup-registration path instead of processing the request immediately. Allocate after-startup requests in a child of the current transaction context, with a reset callback that clears the List and restores the state. Normal error cleanup then handles failures without PG_TRY(). Shared memory already allocated before a failure remains allocated, as shared memory cannot be freed, but the process-local bookkeeping is restored. Extend test_shmem to cover failures while collecting and allocating requests, retried in one session because the state is backend-local. Author: Ayush Tiwari --- src/backend/storage/ipc/shmem.c | 57 +++++++++++++++++-- .../test_shmem/t/001_late_shmem_alloc.pl | 28 +++++++++ .../modules/test_shmem/test_shmem--1.0.sql | 4 ++ src/test/modules/test_shmem/test_shmem.c | 39 +++++++++++++ 4 files changed, 122 insertions(+), 6 deletions(-) diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c index a3d56cf55dd..9986d6a69ce 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" /* @@ -168,6 +169,13 @@ typedef struct static List *pending_shmem_requests; +/* + * During an after-startup request, allocate the request list in a context + * that is reset on error, so that it cannot become a dangling pointer. + */ +static MemoryContext late_shmem_requests_context; +static MemoryContextCallback late_shmem_requests_callback; + /* * Per-process state machine, for sanity checking that we do things in the * right order. @@ -274,6 +282,8 @@ typedef struct static bool firstNumaTouch = true; static void CallShmemCallbacksAfterStartup(const ShmemCallbacks *callbacks); +static void BeginLateShmemRequests(void); +static void ForgetLateShmemRequests(void *arg); static void InitShmemIndexEntry(ShmemRequest *request); static bool AttachShmemIndexEntry(ShmemRequest *request, bool missing_ok); @@ -336,6 +346,7 @@ void ShmemRequestInternal(ShmemStructOpts *options, ShmemRequestKind kind) { ShmemRequest *request; + MemoryContext oldcontext = NULL; /* Check the options */ if (options->name == NULL) @@ -374,10 +385,47 @@ ShmemRequestInternal(ShmemStructOpts *options, ShmemRequestKind kind) } /* Request looks valid, remember it */ + if (late_shmem_requests_context != NULL) + oldcontext = MemoryContextSwitchTo(late_shmem_requests_context); request = palloc(sizeof(ShmemRequest)); request->options = options; request->kind = kind; pending_shmem_requests = lappend(pending_shmem_requests, request); + if (oldcontext != NULL) + MemoryContextSwitchTo(oldcontext); +} + +/* Start collecting an after-startup request. */ +static void +BeginLateShmemRequests(void) +{ + MemoryContext parent = CurTransactionContext != NULL ? + CurTransactionContext : CurrentMemoryContext; + + Assert(pending_shmem_requests == NIL); + Assert(late_shmem_requests_context == NULL); + Assert(late_shmem_requests_callback.func == NULL); + + late_shmem_requests_context = + AllocSetContextCreate(parent, "Pending shmem requests", + ALLOCSET_SMALL_SIZES); + late_shmem_requests_callback.func = ForgetLateShmemRequests; + late_shmem_requests_callback.arg = NULL; + MemoryContextRegisterResetCallback(late_shmem_requests_context, + &late_shmem_requests_callback); +} + +/* Discard an after-startup request and reset the process-local state. */ +static void +ForgetLateShmemRequests(void *arg) +{ + foreach_ptr(ShmemRequest, request, pending_shmem_requests) + pfree(request->options); + + pending_shmem_requests = NIL; + late_shmem_requests_context = NULL; + late_shmem_requests_callback.func = NULL; + shmem_request_state = SRS_DONE; } /* @@ -901,6 +949,7 @@ CallShmemCallbacksAfterStartup(const ShmemCallbacks *callbacks) bool notfound_any; Assert(shmem_request_state == SRS_DONE); + BeginLateShmemRequests(); shmem_request_state = SRS_REQUESTING; /* @@ -914,7 +963,7 @@ CallShmemCallbacksAfterStartup(const ShmemCallbacks *callbacks) if (pending_shmem_requests == NIL) { - shmem_request_state = SRS_DONE; + MemoryContextDelete(late_shmem_requests_context); return; } @@ -952,11 +1001,7 @@ CallShmemCallbacksAfterStartup(const ShmemCallbacks *callbacks) 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) @@ -971,7 +1016,7 @@ CallShmemCallbacksAfterStartup(const ShmemCallbacks *callbacks) } LWLockRelease(ShmemIndexLock); - shmem_request_state = SRS_DONE; + MemoryContextDelete(late_shmem_requests_context); } /* 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..a9481997d52 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 @@ -25,6 +25,34 @@ my $attach_count2 = $node->safe_psql("postgres", "SELECT get_test_shmem_attach_count();"); cmp_ok($attach_count2, '>', $attach_count1, "attach callback is called in each backend"); + +sub try_shmem_failure_twice +{ + my ($mode) = @_; + my $sql = qq[ +DO \$\$ +BEGIN + FOR i IN 1..2 LOOP + BEGIN + PERFORM test_shmem_failure($mode); + EXCEPTION WHEN others THEN + RAISE NOTICE 'attempt %: %', i, SQLERRM; + END; + END LOOP; +END +\$\$;]; + return $node->psql('postgres', $sql); +} + +# The state is backend-local, so the two attempts must share a session. +foreach my $mode (0, 1) +{ + my ($ret, $stdout, $stderr) = try_shmem_failure_twice($mode); + + is($ret, 0, "session survives repeated failing shmem request $mode"); + like($stderr, qr/attempt 1: /, "shmem request $mode fails"); + like($stderr, qr/attempt 2: /, "shmem request $mode fails when retried"); +} $node->stop; ### diff --git a/src/test/modules/test_shmem/test_shmem--1.0.sql b/src/test/modules/test_shmem/test_shmem--1.0.sql index 2d01fd9256c..62d3458fea9 100644 --- a/src/test/modules/test_shmem/test_shmem--1.0.sql +++ b/src/test/modules/test_shmem/test_shmem--1.0.sql @@ -7,3 +7,7 @@ CREATE FUNCTION get_test_shmem_attach_count() RETURNS pg_catalog.int4 STRICT AS 'MODULE_PATHNAME' LANGUAGE C; + +CREATE FUNCTION test_shmem_failure(pg_catalog.int4) +RETURNS pg_catalog.void STRICT +AS 'MODULE_PATHNAME' LANGUAGE C; diff --git a/src/test/modules/test_shmem/test_shmem.c b/src/test/modules/test_shmem/test_shmem.c index 9bd4012b435..348f26877af 100644 --- a/src/test/modules/test_shmem/test_shmem.c +++ b/src/test/modules/test_shmem/test_shmem.c @@ -38,6 +38,7 @@ static bool attached_or_initialized = false; static void test_shmem_request(void *arg); static void test_shmem_init(void *arg); static void test_shmem_attach(void *arg); +static void test_shmem_failure_request(void *arg); static const ShmemCallbacks TestShmemCallbacks = { .flags = SHMEM_CALLBACKS_ALLOW_AFTER_STARTUP, @@ -46,6 +47,13 @@ static const ShmemCallbacks TestShmemCallbacks = { .attach_fn = test_shmem_attach, }; +static const ShmemCallbacks TestShmemFailureCallbacks = { + .flags = SHMEM_CALLBACKS_ALLOW_AFTER_STARTUP, + .request_fn = test_shmem_failure_request, +}; + +static int failure_mode; + static void test_shmem_request(void *arg) { @@ -82,6 +90,28 @@ test_shmem_attach(void *arg) attached_or_initialized = true; } +/* Request callback used by the after-startup failure tests. */ +static void +test_shmem_failure_request(void *arg) +{ + static void *ptr1; + + switch (failure_mode) + { + case 0: + ShmemRequestStruct(.name = "test_shmem callback error area", + .size = 1024, .ptr = &ptr1); + elog(ERROR, "test_shmem request callback failed on purpose"); + case 1: + ShmemRequestStruct(.name = "test_shmem oversized area", + .size = (Size) 1024 * 1024 * 1024, + .ptr = &ptr1); + break; + default: + elog(ERROR, "unrecognized test_shmem failure mode: %d", failure_mode); + } +} + void _PG_init(void) { @@ -89,6 +119,15 @@ _PG_init(void) RegisterShmemCallbacks(&TestShmemCallbacks); } +PG_FUNCTION_INFO_V1(test_shmem_failure); +Datum +test_shmem_failure(PG_FUNCTION_ARGS) +{ + failure_mode = PG_GETARG_INT32(0); + RegisterShmemCallbacks(&TestShmemFailureCallbacks); + PG_RETURN_VOID(); +} + PG_FUNCTION_INFO_V1(get_test_shmem_attach_count); Datum get_test_shmem_attach_count(PG_FUNCTION_ARGS) -- 2.34.1