Subject: Re: Little cleanup of ShmemInit function names

From: Heikki Linnakangas <[email-masked]>
Date: Wed, 28 Aug 2024 20:26:06 +0300

On 28/08/2024 18:26, Andreas Karlsson wrote:
> On 8/7/24 2:08 PM, Heikki Linnakangas wrote:
>> The attached patches rename them to follow the usual naming convention.
>> The InitLocks() function is refactored a bit more, splitting the
>> per-backend initialization to a separate InitLockManagerAccess()
>> function. That's why it's in a separate commit.
> 
> I like the idea behind the patches. And they still apply and build.
> 
> The first patch is clean and well commented. I just have two minor nitpicks.
> 
> Small typo with the extra "which" which makes the sentence not flow
> correctly
> 
> "This is called from CreateSharedMemoryAndSemaphores(), which see for
> more comments."

Hmm, I don't see the issue. It's an uncommon sentence structure, but it 
was there before this patch, and it's correct AFAICS. If you grep for 
"which see ", you'll find some more examples of that.

> On the topic of minor language issues I think the comma below is redundant.
> 
> "In the normal postmaster case, the shared hash tables are created here."

On second thoughts, I rearranged the sentences in the paragraph a 
little, see attached.

> The second patch is a simple renaming which reduces mental load by
> making the naming more consistent so I like it. Also since these
> functions are not really useful for any extension authors I do not see
> any harm in renaming them.
> 
> After cleaning up the language of that comment I think these patches can
> be committed.

Thanks for the review!

-- 
Heikki Linnakangas
Neon (https://neon.tech)
From 337b36cc820d19dd626ad49930e16b8bc7dbf619 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <[email-masked]>
Date: Wed, 28 Aug 2024 20:19:20 +0300
Subject: [PATCH v2 1/2] Refactor lock manager initialization to make it a bit
 less special

Split the shared and local initialization to separate functions, and
follow the common naming conventions. With this, we no longer create
the LockMethodLocalHash hash table in the postmaster process, which
was always pointless.

Reviewed-by: Andreas Karlsson
Discussion: https://www.postgresql.org/message-id/[email-masked]
---
 src/backend/storage/ipc/ipci.c    |  4 ++--
 src/backend/storage/lmgr/lock.c   | 35 +++++++++++++++----------------
 src/backend/utils/init/postinit.c |  3 +++
 src/include/storage/lock.h        |  5 +++--
 4 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index 35fa2e1dda..8f8a89124e 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -116,7 +116,7 @@ CalculateShmemSize(int *num_semaphores)
 	size = add_size(size, dsm_estimate_size());
 	size = add_size(size, DSMRegistryShmemSize());
 	size = add_size(size, BufferShmemSize());
-	size = add_size(size, LockShmemSize());
+	size = add_size(size, LockManagerShmemSize());
 	size = add_size(size, PredicateLockShmemSize());
 	size = add_size(size, ProcGlobalShmemSize());
 	size = add_size(size, XLogPrefetchShmemSize());
@@ -291,7 +291,7 @@ CreateOrAttachShmemStructs(void)
 	/*
 	 * Set up lock manager
 	 */
-	InitLocks();
+	LockManagerShmemInit();
 
 	/*
 	 * Set up predicate lock manager
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index 0400a50777..6dbc41dae7 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -377,19 +377,17 @@ static void GetSingleProcBlockerStatusData(PGPROC *blocked_proc,

 /*
- * InitLocks -- Initialize the lock manager's data structures.
+ * Initialize the lock manager's shmem data structures.
  *
- * This is called from CreateSharedMemoryAndSemaphores(), which see for
- * more comments.  In the normal postmaster case, the shared hash tables
- * are created here, as well as a locallock hash table that will remain
- * unused and empty in the postmaster itself.  Backends inherit the pointers
- * to the shared tables via fork(), and also inherit an image of the locallock
- * hash table, which they proceed to use.  In the EXEC_BACKEND case, each
- * backend re-executes this code to obtain pointers to the already existing
- * shared hash tables and to create its locallock hash table.
+ * This is called from CreateSharedMemoryAndSemaphores(), which see for more
+ * comments.  In the normal postmaster case, the shared hash tables are
+ * created here, and backends inherit pointers to them via fork().  In the
+ * EXEC_BACKEND case, each backend re-executes this code to obtain pointers to
+ * the already existing shared hash tables.  In either case, each backend must
+ * also call InitLockManagerAccess() to create the locallock hash table.
  */
 void
-InitLocks(void)
+LockManagerShmemInit(void)
 {
 	HASHCTL		info;
 	long		init_table_size,
@@ -444,18 +442,19 @@ InitLocks(void)
 						sizeof(FastPathStrongRelationLockData), &found);
 	if (!found)
 		SpinLockInit(&FastPathStrongRelationLocks->mutex);
+}
 
+/*
+ * Initialize the lock manager's backend-private data structures.
+ */
+void
+InitLockManagerAccess(void)
+{
 	/*
 	 * Allocate non-shared hash table for LOCALLOCK structs.  This stores lock
 	 * counts and resource owner information.
-	 *
-	 * The non-shared table could already exist in this process (this occurs
-	 * when the postmaster is recreating shared memory after a backend crash).
-	 * If so, delete and recreate it.  (We could simply leave it, since it
-	 * ought to be empty in the postmaster, but for safety let's zap it.)
 	 */
-	if (LockMethodLocalHash)
-		hash_destroy(LockMethodLocalHash);
+	HASHCTL		info;
 
 	info.keysize = sizeof(LOCALLOCKTAG);
 	info.entrysize = sizeof(LOCALLOCK);
@@ -3571,7 +3570,7 @@ PostPrepare_Locks(TransactionId xid)
  * Estimate shared-memory space used for lock tables
  */
 Size
-LockShmemSize(void)
+LockManagerShmemSize(void)
 {
 	Size		size = 0;
 	long		max_table_size;
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 13524ea488..04a47d6e70 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -606,6 +606,9 @@ BaseInit(void)
 	 */
 	InitXLogInsert();
 
+	/* Initialize lock manager's local structs */
+	InitLockManagerAccess();
+
 	/*
 	 * Initialize replication slots after pgstat. The exit hook might need to
 	 * drop ephemeral slots, which in turn triggers stats reporting.
diff --git a/src/include/storage/lock.h b/src/include/storage/lock.h
index cc1f6e78c3..8b328a06d9 100644
--- a/src/include/storage/lock.h
+++ b/src/include/storage/lock.h
@@ -544,7 +544,9 @@ typedef enum
 /*
  * function prototypes
  */
-extern void InitLocks(void);
+extern void LockManagerShmemInit(void);
+extern Size LockManagerShmemSize(void);
+extern void InitLockManagerAccess(void);
 extern LockMethod GetLocksMethodTable(const LOCK *lock);
 extern LockMethod GetLockTagsMethodTable(const LOCKTAG *locktag);
 extern uint32 LockTagHashCode(const LOCKTAG *locktag);
@@ -584,7 +586,6 @@ extern bool LockCheckConflicts(LockMethod lockMethodTable,
 extern void GrantLock(LOCK *lock, PROCLOCK *proclock, LOCKMODE lockmode);
 extern void GrantAwaitedLock(void);
 extern void RemoveFromWaitQueue(PGPROC *proc, uint32 hashcode);
-extern Size LockShmemSize(void);
 extern LockData *GetLockStatusData(void);
 extern BlockedProcsData *GetBlockerStatusData(int blocked_pid);
 
-- 
2.39.2

From 00d261563b4941105459e7d2393386cff47de12b Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <[email-masked]>
Date: Wed, 28 Aug 2024 20:20:50 +0300
Subject: [PATCH v2 2/2] Rename some shared memory initialization routines

To make them follow the usual naming convention where
FoobarShmemSize() calculates the amount of shared memory needed by
Foobar subsystem, and FoobarShmemInit() performs the initialization.

I didn't rename CreateLWLocks() and InitShmmeIndex(), because they are
a little special. They need to be called before any of the other
ShmemInit() functions, because they set up the shared memory
bookkeeping itself. I also didn't rename InitProcGlobal(), because
unlike other Shmeminit functions, it's not called by individual
backends.

Reviewed-by: Andreas Karlsson
Discussion: https://www.postgresql.org/message-id/[email-masked]
---
 src/backend/storage/buffer/buf_init.c       |  6 +++---
 src/backend/storage/buffer/bufmgr.c         |  2 +-
 src/backend/storage/buffer/freelist.c       |  2 +-
 src/backend/storage/ipc/ipci.c              | 14 +++++++-------
 src/backend/storage/ipc/procarray.c         |  4 ++--
 src/backend/storage/ipc/sinvaladt.c         | 10 +++++-----
 src/backend/storage/lmgr/predicate.c        |  6 +++---
 src/backend/utils/activity/backend_status.c |  4 ++--
 src/backend/utils/init/postinit.c           |  2 +-
 src/include/storage/bufmgr.h                |  6 +++---
 src/include/storage/predicate.h             |  2 +-
 src/include/storage/procarray.h             |  2 +-
 src/include/storage/sinvaladt.h             |  4 ++--
 src/include/utils/backend_status.h          |  2 +-
 14 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/src/backend/storage/buffer/buf_init.c b/src/backend/storage/buffer/buf_init.c
index 46116a1f64..09bec6449b 100644
--- a/src/backend/storage/buffer/buf_init.c
+++ b/src/backend/storage/buffer/buf_init.c
@@ -65,7 +65,7 @@ CkptSortItem *CkptBufferIds;
  * postmaster, or in a standalone backend).
  */
 void
-InitBufferPool(void)
+BufferManagerShmemInit(void)
 {
 	bool		foundBufs,
 				foundDescs,
@@ -151,13 +151,13 @@ InitBufferPool(void)
 }
 
 /*
- * BufferShmemSize
+ * BufferManagerShmemSize
  *
  * compute the size of shared memory for the buffer pool including
  * data pages, buffer descriptors, hash tables, etc.
  */
 Size
-BufferShmemSize(void)
+BufferManagerShmemSize(void)
 {
 	Size		size = 0;
 
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index fb38c7bdd4..5cdd2f10fc 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -3582,7 +3582,7 @@ AtEOXact_Buffers(bool isCommit)
  * buffer pool.
  */
 void
-InitBufferPoolAccess(void)
+InitBufferManagerAccess(void)
 {
 	HASHCTL		hash_ctl;
 
diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c
index 19797de31a..dffdd57e9b 100644
--- a/src/backend/storage/buffer/freelist.c
+++ b/src/backend/storage/buffer/freelist.c
@@ -506,7 +506,7 @@ StrategyInitialize(bool init)
 
 		/*
 		 * Grab the whole linked list of free buffers for our strategy. We
-		 * assume it was previously set up by InitBufferPool().
+		 * assume it was previously set up by BufferManagerShmemInit().
 		 */
 		StrategyControl->firstFreeBuffer = 0;
 		StrategyControl->lastFreeBuffer = NBuffers - 1;
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index 8f8a89124e..6caeca3a8e 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -115,7 +115,7 @@ CalculateShmemSize(int *num_semaphores)
 											 sizeof(ShmemIndexEnt)));
 	size = add_size(size, dsm_estimate_size());
 	size = add_size(size, DSMRegistryShmemSize());
-	size = add_size(size, BufferShmemSize());
+	size = add_size(size, BufferManagerShmemSize());
 	size = add_size(size, LockManagerShmemSize());
 	size = add_size(size, PredicateLockShmemSize());
 	size = add_size(size, ProcGlobalShmemSize());
@@ -132,7 +132,7 @@ CalculateShmemSize(int *num_semaphores)
 	size = add_size(size, LWLockShmemSize());
 	size = add_size(size, ProcArrayShmemSize());
 	size = add_size(size, BackendStatusShmemSize());
-	size = add_size(size, SInvalShmemSize());
+	size = add_size(size, SharedInvalShmemSize());
 	size = add_size(size, PMSignalShmemSize());
 	size = add_size(size, ProcSignalShmemSize());
 	size = add_size(size, CheckpointerShmemSize());
@@ -286,7 +286,7 @@ CreateOrAttachShmemStructs(void)
 	CommitTsShmemInit();
 	SUBTRANSShmemInit();
 	MultiXactShmemInit();
-	InitBufferPool();
+	BufferManagerShmemInit();
 
 	/*
 	 * Set up lock manager
@@ -296,22 +296,22 @@ CreateOrAttachShmemStructs(void)
 	/*
 	 * Set up predicate lock manager
 	 */
-	InitPredicateLocks();
+	PredicateLockShmemInit();
 
 	/*
 	 * Set up process table
 	 */
 	if (!IsUnderPostmaster)
 		InitProcGlobal();
-	CreateSharedProcArray();
-	CreateSharedBackendStatus();
+	ProcArrayShmemInit();
+	BackendStatusShmemInit();
 	TwoPhaseShmemInit();
 	BackgroundWorkerShmemInit();
 
 	/*
 	 * Set up shared-inval messaging
 	 */
-	CreateSharedInvalidationState();
+	SharedInvalShmemInit();
 
 	/*
 	 * Set up interprocess signaling mechanisms
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index af3b15e93d..36610a1c7e 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -370,7 +370,7 @@ static inline FullTransactionId FullXidRelativeTo(FullTransactionId rel,
 static void GlobalVisUpdateApply(ComputeXidHorizonsResult *horizons);
 
 /*
- * Report shared-memory space needed by CreateSharedProcArray.
+ * Report shared-memory space needed by ProcArrayShmemInit
  */
 Size
 ProcArrayShmemSize(void)
@@ -415,7 +415,7 @@ ProcArrayShmemSize(void)
  * Initialize the shared PGPROC array during postmaster startup.
  */
 void
-CreateSharedProcArray(void)
+ProcArrayShmemInit(void)
 {
 	bool		found;
 
diff --git a/src/backend/storage/ipc/sinvaladt.c b/src/backend/storage/ipc/sinvaladt.c
index b486d8ddd1..db59637c43 100644
--- a/src/backend/storage/ipc/sinvaladt.c
+++ b/src/backend/storage/ipc/sinvaladt.c
@@ -212,10 +212,10 @@ static void CleanupInvalidationState(int status, Datum arg);

 /*
- * SInvalShmemSize --- return shared-memory space needed
+ * SharedInvalShmemSize --- return shared-memory space needed
  */
 Size
-SInvalShmemSize(void)
+SharedInvalShmemSize(void)
 {
 	Size		size;
 
@@ -227,18 +227,18 @@ SInvalShmemSize(void)
 }
 
 /*
- * CreateSharedInvalidationState
+ * SharedInvalShmemInit
  *		Create and initialize the SI message buffer
  */
 void
-CreateSharedInvalidationState(void)
+SharedInvalShmemInit(void)
 {
 	int			i;
 	bool		found;
 
 	/* Allocate space in shared memory */
 	shmInvalBuffer = (SISeg *)
-		ShmemInitStruct("shmInvalBuffer", SInvalShmemSize(), &found);
+		ShmemInitStruct("shmInvalBuffer", SharedInvalShmemSize(), &found);
 	if (found)
 		return;
 
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index b455b78f9f..e24a0f2fdb 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -153,7 +153,7 @@
  * INTERFACE ROUTINES
  *
  * housekeeping for setting up shared memory predicate lock structures
- *		InitPredicateLocks(void)
+ *		PredicateLockShmemInit(void)
  *		PredicateLockShmemSize(void)
  *
  * predicate lock reporting
@@ -1127,7 +1127,7 @@ CheckPointPredicate(void)
 /*------------------------------------------------------------------------*/
 
 /*
- * InitPredicateLocks -- Initialize the predicate locking data structures.
+ * PredicateLockShmemInit -- Initialize the predicate locking data structures.
  *
  * This is called from CreateSharedMemoryAndSemaphores(), which see for
  * more comments.  In the normal postmaster case, the shared hash tables
@@ -1137,7 +1137,7 @@ CheckPointPredicate(void)
  * shared hash tables.
  */
 void
-InitPredicateLocks(void)
+PredicateLockShmemInit(void)
 {
 	HASHCTL		info;
 	long		max_table_size;
diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c
index 1ccf4c6d83..34a55e2177 100644
--- a/src/backend/utils/activity/backend_status.c
+++ b/src/backend/utils/activity/backend_status.c
@@ -77,7 +77,7 @@ static void pgstat_setup_backend_status_context(void);

 /*
- * Report shared-memory space needed by CreateSharedBackendStatus.
+ * Report shared-memory space needed by BackendStatusShmemInit.
  */
 Size
 BackendStatusShmemSize(void)
@@ -113,7 +113,7 @@ BackendStatusShmemSize(void)
  * during postmaster startup.
  */
 void
-CreateSharedBackendStatus(void)
+BackendStatusShmemInit(void)
 {
 	Size		size;
 	bool		found;
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 04a47d6e70..3b50ce19a2 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -592,7 +592,7 @@ BaseInit(void)
 	/* Do local initialization of storage and buffer managers */
 	InitSync();
 	smgrinit();
-	InitBufferPoolAccess();
+	InitBufferManagerAccess();
 
 	/*
 	 * Initialize temporary file access after pgstat, so that the temporary
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index c8422571b7..eb0fba4230 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -249,7 +249,7 @@ extern Buffer ExtendBufferedRelTo(BufferManagerRelation bmr,
 								  BlockNumber extend_to,
 								  ReadBufferMode mode);
 
-extern void InitBufferPoolAccess(void);
+extern void InitBufferManagerAccess(void);
 extern void AtEOXact_Buffers(bool isCommit);
 extern char *DebugPrintBufferRefcount(Buffer buffer);
 extern void CheckPointBuffers(int flags);
@@ -300,8 +300,8 @@ extern void LimitAdditionalLocalPins(uint32 *additional_pins);
 extern bool EvictUnpinnedBuffer(Buffer buf);
 
 /* in buf_init.c */
-extern void InitBufferPool(void);
-extern Size BufferShmemSize(void);
+extern void BufferManagerShmemInit(void);
+extern Size BufferManagerShmemSize(void);
 
 /* in localbuf.c */
 extern void AtProcExit_LocalBuffers(void);
diff --git a/src/include/storage/predicate.h b/src/include/storage/predicate.h
index 14ee9b94a2..d5c14986e2 100644
--- a/src/include/storage/predicate.h
+++ b/src/include/storage/predicate.h
@@ -37,7 +37,7 @@ typedef void *SerializableXactHandle;
  */
 
 /* housekeeping for shared memory predicate lock structures */
-extern void InitPredicateLocks(void);
+extern void PredicateLockShmemInit(void);
 extern Size PredicateLockShmemSize(void);
 
 extern void CheckPointPredicate(void);
diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h
index 8ca6050462..56af0b40b3 100644
--- a/src/include/storage/procarray.h
+++ b/src/include/storage/procarray.h
@@ -21,7 +21,7 @@

 extern Size ProcArrayShmemSize(void);
-extern void CreateSharedProcArray(void);
+extern void ProcArrayShmemInit(void);
 extern void ProcArrayAdd(PGPROC *proc);
 extern void ProcArrayRemove(PGPROC *proc, TransactionId latestXid);
 
diff --git a/src/include/storage/sinvaladt.h b/src/include/storage/sinvaladt.h
index c3c97b3f8b..c9d175b634 100644
--- a/src/include/storage/sinvaladt.h
+++ b/src/include/storage/sinvaladt.h
@@ -28,8 +28,8 @@
 /*
  * prototypes for functions in sinvaladt.c
  */
-extern Size SInvalShmemSize(void);
-extern void CreateSharedInvalidationState(void);
+extern Size SharedInvalShmemSize(void);
+extern void SharedInvalShmemInit(void);
 extern void SharedInvalBackendInit(bool sendOnly);
 
 extern void SIInsertDataEntries(const SharedInvalidationMessage *data, int n);
diff --git a/src/include/utils/backend_status.h b/src/include/utils/backend_status.h
index 7b7f6f59d0..97874300c3 100644
--- a/src/include/utils/backend_status.h
+++ b/src/include/utils/backend_status.h
@@ -299,7 +299,7 @@ extern PGDLLIMPORT PgBackendStatus *MyBEEntry;
  * ----------
  */
 extern Size BackendStatusShmemSize(void);
-extern void CreateSharedBackendStatus(void);
+extern void BackendStatusShmemInit(void);

 /* ----------
-- 
2.39.2