From ba55c513a80b3eae391359893f3bbdeb055e4ba6 Mon Sep 17 00:00:00 2001
From: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
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 <ayushtiwari.slg01@gmail.com>
---
 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.
      </para>
+     <para>
+        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 <function>init_fn</function> did not complete.
+        Subsystems must detect that case and not use incompletely initialized
+        shared state.
+     </para>
     </sect3>
 
     <sect3 id="xfunc-shared-addin-dynamic">
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 <limits.h>
+
 #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

