0006-Add-TODOs-and-questions-about-previous-comm-20250113.patch

From f33d7888253650c9f10634c8c28ea10c2e3d0fd8 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Date: Mon, 6 Jan 2025 14:40:51 +0530
Subject: [PATCH 6/7] Add TODOs and questions about previous commits

The commit just marks the places which need more work or whether the
code raises some questions. This is not an exhaustive list of TODOs.
More TODOs may come up as I work with these patches further.

Ashutosh Bapat
---
 src/backend/storage/buffer/buf_init.c         |  9 +++++---
 src/backend/storage/ipc/ipc.c                 |  4 ++++
 src/backend/storage/ipc/ipci.c                | 11 +++++++++-
 src/backend/storage/ipc/shmem.c               | 22 ++++++++++++++++---
 src/backend/storage/lmgr/lwlock.c             |  5 +++++
 src/backend/tcop/postgres.c                   |  6 +++++
 .../utils/activity/wait_event_names.txt       |  1 +
 src/include/storage/buf_internals.h           |  5 +++++
 src/include/storage/pg_shmem.h                | 18 ++++++++++-----
 9 files changed, 69 insertions(+), 12 deletions(-)

diff --git a/src/backend/storage/buffer/buf_init.c b/src/backend/storage/buffer/buf_init.c
index ae58f82937f..bf74b4b01a7 100644
--- a/src/backend/storage/buffer/buf_init.c
+++ b/src/backend/storage/buffer/buf_init.c
@@ -155,8 +155,11 @@ BufferManagerShmemInit(void)
 
 /*
  * Reinitialize shared memory structures, which size depends on NBuffers. It's
- * similar to BufferManagerShmemInit, but applied only to the buffers in the range
- * between NBuffersOld and NBuffers.
+ * similar to BufferManagerShmemInit, but applied only to the buffers in the
+ * range between NBuffersOld and NBuffers.
+ *
+ * TODO: Avoid code duplication with BufferManagerShmemInit() and also assess
+ * which functionality in the latter is required in this function.
  */
 void
 BufferManagerShmemResize(int NBuffersOld)
@@ -255,7 +258,7 @@ BufferManagerShmemSize(int shmem_slot)
 
 	if (shmem_slot == MAIN_SHMEM_SLOT)
 		return size;
- 
+
 	if (shmem_slot == BUFFER_DESCRIPTORS_SHMEM_SLOT)
 	{
 		/* size of buffer descriptors */
diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index 2aabd4a77f3..556eb469f4f 100644
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -68,6 +68,10 @@ static void proc_exit_prepare(int code);
  * ----------------------------------------------------------------
  */
 
+/*
+ * TODO: Why do we need to increase this by 20? I didn't notice any new calls to
+ * on_shmem_exit or on_proc_exit or before_shmem_exit.
+ */
 #define MAX_ON_EXITS 40
 
 struct ONEXIT
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index 15d06fd4ca4..e076f96ebf2 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -204,6 +204,12 @@ AttachSharedMemoryStructs(void)
 /*
  * CreateSharedMemoryAndSemaphores
  *		Creates and initializes shared memory and semaphores.
+ *
+ * TODO: IMO this function should be rewritten to calculate the size of each
+ * shared memory slot or mapping. Instead of passing slot number to
+ * CalculateShmemSize, we should instead let each shared memory module use their
+ * own slot number and update the required sizes in the corresponding mapping.
+ * Then allocate shared memory in each of the mappings.
  */
 void
 CreateSharedMemoryAndSemaphores(void)
@@ -222,7 +228,10 @@ CreateSharedMemoryAndSemaphores(void)
 		elog(DEBUG3, "invoking IpcMemoryCreate(size=%zu)", size);
 
 		/*
-		 * Create the shmem segment
+		 * Create the shmem segment.
+		 * TODO: while each slot will return a different shim, only the last one
+		 * is passed to dsm_postmaster_startup(). Is that right? Shouldn't we
+		 * pass all of them or none.
 		 */
 		seghdr = PGSharedMemoryCreate(size, &shim);
 
diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c
index faca7c9a525..c1dde378329 100644
--- a/src/backend/storage/ipc/shmem.c
+++ b/src/backend/storage/ipc/shmem.c
@@ -82,6 +82,7 @@ static void *ShmemAllocRawInSlot(Size size, Size *allocated_size,
 
 ShmemSegment Segments[ANON_MAPPINGS];
 
+/*TODO: shouldn't this be part of the ShmemSegment structure? */
 static HTAB *ShmemIndex = NULL; /* primary index hashtable for shmem */
 
 
@@ -490,9 +491,18 @@ ShmemInitStructInSlot(const char *name, Size size, bool *foundPtr,
 	{
 		/*
 		 * Structure is in the shmem index so someone else has allocated it
-		 * already. Verify the structure's size:
-		 * - If it's the same, we've found the expected structure.
-		 * - If it's different, we're resizing the expected structure.
+		 * already. Verify the structure's size: - If it's the same, we've found
+		 * the expected structure.  - If it's different, we're resizing the
+		 * expected structure.
+		 *
+		 * TODO: This works because every structure that needs to be resized
+		 * resides in a shmem slot by itself. But it won't work if a slot
+		 * contains more structures, that need to be resized, placed in adjacent
+		 * memory. Also we are not updating the Shmem stats like freeoffset. I
+		 * think we will keep all resizable structures in a slot for themselves,
+		 * and not have a hash table in such slots since resizing the hash table
+		 * itself might cause memory to be allocated next to the resizable
+		 * structure making it difficult to resize it.
 		 */
 		if (result->size != size)
 			result->size = size;
@@ -584,6 +594,12 @@ pg_get_shmem_allocations(PG_FUNCTION_ARGS)
 
 	hash_seq_init(&hstat, ShmemIndex);
 
+	/*
+	 * TODO: For the sake of completeness we should rotate through all the slots
+	 * (after saving slotwise ShmemIndex, if any). Do we want to also output
+	 * shmem slot name, but that would expose the slotified structure of shared
+	 * memory.
+	 */
 	/* output all allocated entries */
 	memset(nulls, 0, sizeof(nulls));
 	while ((ent = (ShmemIndexEnt *) hash_seq_search(&hstat)) != NULL)
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index cd3237b3736..0be59074709 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -608,6 +608,11 @@ LWLockNewTrancheId(void)
 
 	LWLockCounter = (int *) ((char *) MainLWLockArray - sizeof(int));
 	/* We use the ShmemLock spinlock to protect LWLockCounter */
+	/*
+	 * TODO: We have retained ShmemLock global variable, should we use it here
+	 * instead of main segment lock? We will need spinlock init on the global
+	 * one if yes.
+	 */
 	SpinLockAcquire(Segments[MAIN_SHMEM_SLOT].ShmemLock);
 	result = (*LWLockCounter)++;
 	SpinLockRelease(Segments[MAIN_SHMEM_SLOT].ShmemLock);
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 85902788181..8c89928203b 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -4656,6 +4656,12 @@ PostgresMain(const char *dbname, const char *username)
 		/*
 		 * (6) check for any other interesting events that happened while we
 		 * slept.
+		 * TODO: When a backend is waiting for a command, it won't reload
+		 * configuration and hence wouldn't notice change in shared_buffers. The
+		 * change is only noticed after the command is received and the control
+		 * comes here. We may need to improve this in case we want to resize
+		 * shared buffers or perform of part of that operation in assign_hook
+		 * implementation (e.g. AnonymousShmemResize()).
 		 */
 		if (ConfigReloadPending)
 		{
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index e8ecff5f7f0..acd94c3616c 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -345,6 +345,7 @@ WALSummarizer	"Waiting to read or update WAL summarization state."
 DSMRegistry	"Waiting to read or update the dynamic shared memory registry."
 InjectionPoint	"Waiting to read or update information related to injection points."
 SerialControl	"Waiting to read or update shared <filename>pg_serial</filename> state."
+# TODO, not used anywhere, do we need it?
 ShmemResize	"Waiting to resize shared memory."
 
 #
diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h
index b25dc0199b8..6a352d3942e 100644
--- a/src/include/storage/buf_internals.h
+++ b/src/include/storage/buf_internals.h
@@ -22,6 +22,11 @@
 #include "storage/condition_variable.h"
 #include "storage/lwlock.h"
 #include "storage/shmem.h"
+/*
+ * TODO: this header files doesn't use anything in pg_shmem.h but the files which
+ * include this file may. We should include pg_shmem.h in those files rather than
+ * here.
+ */
 #include "storage/pg_shmem.h"
 #include "storage/smgr.h"
 #include "storage/spin.h"
diff --git a/src/include/storage/pg_shmem.h b/src/include/storage/pg_shmem.h
index c1a96240d79..39521208fb9 100644
--- a/src/include/storage/pg_shmem.h
+++ b/src/include/storage/pg_shmem.h
@@ -42,6 +42,10 @@ typedef struct PGShmemHeader	/* standard header for all Postgres shmem */
 #endif
 } PGShmemHeader;
 
+/*
+ * TODO: should we define it in shmem.c where the previous global variables were
+ * declared? Do we need this structure outside shmem.c?
+ */
 typedef struct ShmemSegment
 {
 	PGShmemHeader *ShmemSegHdr; 	/* shared mem segment header */
@@ -51,11 +55,6 @@ typedef struct ShmemSegment
 									 * allocation */
 } ShmemSegment;
 
-// Number of available slots for anonymous memory mappings
-#define ANON_MAPPINGS 6
-
-extern PGDLLIMPORT ShmemSegment Segments[ANON_MAPPINGS];
-
 /* GUC variables */
 extern PGDLLIMPORT int shared_memory_type;
 extern PGDLLIMPORT int huge_pages;
@@ -111,6 +110,10 @@ extern void AnonymousShmemResize(int newval, void *extra);
  * To be able to dynamically resize largest parts of the data stored in shared
  * memory, we split it into multiple shared memory mappings slots. Each slot
  * contains only certain part of the data, which size depends on NBuffers.
+ *
+ * TODO: convert this into an enum with a sentinel symbol ANON_MAPPINGS, which
+ * itself should be renamed to NUM_ANON_MAPPINGS or NUM_SHMEM_SEGMENTS or
+ * something that indicates that it's the number of shared memory segments.
  */
 
 /* The main slot, contains everything except buffer blocks and related data. */
@@ -131,4 +134,9 @@ extern void AnonymousShmemResize(int newval, void *extra);
 /* Buffer strategy status */
 #define STRATEGY_SHMEM_SLOT 5
 
+// Number of available slots for anonymous memory mappings
+#define ANON_MAPPINGS 6
+
+extern PGDLLIMPORT ShmemSegment Segments[ANON_MAPPINGS];
+
 #endif							/* PG_SHMEM_H */
-- 
2.34.1