Subject: Re: scalability bottlenecks with (many) partitions (and more)

From: Tomas Vondra <[email-masked]>
Date: Fri, 13 Sep 2024 01:44:58 +0200

Turns out there was a bug in EXEC_BACKEND mode, causing failures on the
Windows machine in CI. AFAIK the reason is pretty simple - the backends
don't see the number of fast-path groups postmaster calculated from
max_locks_per_transaction.

Fixed that by calculating it again in AttachSharedMemoryStructs, which
seems to have done the trick. With this the CI builds pass just fine,
but I'm not sure if EXEC_BACKENDS may have some other issues with the
PGPROC changes. Could it happen that the shared memory gets mapped
differently, in which case the pointers might need to be adjusted?

regards

-- 
Tomas Vondra

From 45a2111c51016386a31d766700b23d9d88ff6c0b Mon Sep 17 00:00:00 2001
From: Tomas Vondra <[email-masked]>
Date: Thu, 12 Sep 2024 23:09:41 +0200
Subject: [PATCH v20240913 1/2] Increase the number of fast-path lock slots

The fast-path locking introduced in 9.2 allowed each backend to acquire
up to 16 relation locks cheaply, provided the lock level allows that.
If a backend needs to hold more locks, it has to insert them into the
regular lock table in shared memory. This is considerably more
expensive, and on many-core systems may be subject to contention.

The limit of 16 entries was always rather low, even with simple queries
and schemas with only a few tables. We have to lock all relations - not
just tables, but also indexes, views, etc. Moreover, for planning we
need to lock all relations that might be used in the plan, not just
those that actually get used in the final plan. It only takes a couple
tables with multiple indexes to need more than 16 locks. It was quite
common to fill all fast-path slots.

As partitioning gets used more widely, with more and more partitions,
this limit is trivial to hit, with complex queries easily using hundreds
or even thousands of locks. For workloads doing a lot of I/O this is not
noticeable, but on large machines with enough RAM to keep the data in
memory, the access to the shared lock table may be a serious issue.

This patch improves this by increasing the number of fast-path slots
from 16 to 1024. The slots remain in PGPROC, and are organized as an
array of 16-slot groups (each group being effectively a clone of the
original fast-path approach). Instead of accessing this as a big hash
table with open addressing, we treat this as a 16-way set associative
cache. Each relation (identified by a "relid" OID) is mapped to a
particular 16-slot group by calculating a hash

    h(relid) = ((relid * P) mod N)

where P is a hard-coded prime, and N is the number of groups. This is
not a great hash function, but it works well enough - the main purpose
is to prevent "hot groups" with runs of consecutive OIDs, which might
fill some of the fast-path groups. The multiplication by P ensures that.
If the OIDs are already spread out, the hash should not group them.

The groups are processed by linear search. With only 16 entries this is
cheap, and the groups have very good locality.

Treating this as a simple hash table with open addressing would not be
efficient, especially once the hash table is getting almost full. The
usual solution is to grow the table, but for hash tables in shared
memory that's not trivial. It would also have worse locality, due to
more random access.

Luckily, fast-path locking already has a simple solution to deal with a
full hash table. The lock can be simply inserted into the shared lock
table, just like before. Of course, if this happens too often, that
reduces the benefit of fast-path locking.

This patch hard-codes the number of groups to 64, which means 1024
fast-path locks. As all the information is still stored in PGPROC, this
grows PGPROC by about 4.5kB (from ~840B to ~5kB). This is a trade off
exchanging memory for cheaper locking.

Ultimately, the number of fast-path slots should not be hard coded, but
adjustable based on what the workload does, perhaps using a GUC. That
however means it can't be stored in PGPROC directly.
---
 src/backend/storage/lmgr/lock.c | 118 ++++++++++++++++++++++++++------
 src/include/storage/proc.h      |   8 ++-
 2 files changed, 102 insertions(+), 24 deletions(-)

diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index 83b99a98f08..d053ae0c409 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -167,7 +167,7 @@ typedef struct TwoPhaseLockRecord
  * our locks to the primary lock table, but it can never be lower than the
  * real value, since only we can acquire locks on our own behalf.
  */
-static int	FastPathLocalUseCount = 0;
+static int	FastPathLocalUseCounts[FP_LOCK_GROUPS_PER_BACKEND];
 
 /*
  * Flag to indicate if the relation extension lock is held by this backend.
@@ -184,23 +184,53 @@ static int	FastPathLocalUseCount = 0;
  */
 static bool IsRelationExtensionLockHeld PG_USED_FOR_ASSERTS_ONLY = false;
 
+/*
+ * Macros to calculate the group and index for a relation.
+ *
+ * The formula is a simple hash function, designed to spread the OIDs a bit,
+ * so that even contiguous values end up in different groups. In most cases
+ * there will be gaps anyway, but the multiplication should help a bit.
+ *
+ * The selected value (49157) is a prime not too close to 2^k, and it's
+ * small enough to not cause overflows (in 64-bit).
+ */
+#define FAST_PATH_LOCK_REL_GROUP(rel) \
+	(((uint64) (rel) * 49157) % FP_LOCK_GROUPS_PER_BACKEND)
+
+/* Calculate index in the whole per-backend array of lock slots. */
+#define FP_LOCK_SLOT_INDEX(group, index) \
+	(AssertMacro(((group) >= 0) && ((group) < FP_LOCK_GROUPS_PER_BACKEND)), \
+	 AssertMacro(((index) >= 0) && ((index) < FP_LOCK_SLOTS_PER_GROUP)), \
+	 ((group) * FP_LOCK_SLOTS_PER_GROUP + (index)))
+
+/*
+ * Given a lock index (into the per-backend array), calculated using the
+ * FP_LOCK_SLOT_INDEX macro, calculate group and index (within the group).
+ */
+#define FAST_PATH_LOCK_GROUP(index)	\
+	(AssertMacro(((index) >= 0) && ((index) < FP_LOCK_SLOTS_PER_BACKEND)), \
+	 ((index) / FP_LOCK_SLOTS_PER_GROUP))
+#define FAST_PATH_LOCK_INDEX(index)	\
+	(AssertMacro(((index) >= 0) && ((index) < FP_LOCK_SLOTS_PER_BACKEND)), \
+	 ((index) % FP_LOCK_SLOTS_PER_GROUP))
+
 /* Macros for manipulating proc->fpLockBits */
 #define FAST_PATH_BITS_PER_SLOT			3
 #define FAST_PATH_LOCKNUMBER_OFFSET		1
 #define FAST_PATH_MASK					((1 << FAST_PATH_BITS_PER_SLOT) - 1)
 #define FAST_PATH_GET_BITS(proc, n) \
-	(((proc)->fpLockBits >> (FAST_PATH_BITS_PER_SLOT * n)) & FAST_PATH_MASK)
+	(((proc)->fpLockBits[(n)/16] >> (FAST_PATH_BITS_PER_SLOT * FAST_PATH_LOCK_INDEX(n))) & FAST_PATH_MASK)
 #define FAST_PATH_BIT_POSITION(n, l) \
 	(AssertMacro((l) >= FAST_PATH_LOCKNUMBER_OFFSET), \
 	 AssertMacro((l) < FAST_PATH_BITS_PER_SLOT+FAST_PATH_LOCKNUMBER_OFFSET), \
 	 AssertMacro((n) < FP_LOCK_SLOTS_PER_BACKEND), \
-	 ((l) - FAST_PATH_LOCKNUMBER_OFFSET + FAST_PATH_BITS_PER_SLOT * (n)))
+	 ((l) - FAST_PATH_LOCKNUMBER_OFFSET + FAST_PATH_BITS_PER_SLOT * (FAST_PATH_LOCK_INDEX(n))))
 #define FAST_PATH_SET_LOCKMODE(proc, n, l) \
-	 (proc)->fpLockBits |= UINT64CONST(1) << FAST_PATH_BIT_POSITION(n, l)
+	 (proc)->fpLockBits[FAST_PATH_LOCK_GROUP(n)] |= UINT64CONST(1) << FAST_PATH_BIT_POSITION(n, l)
 #define FAST_PATH_CLEAR_LOCKMODE(proc, n, l) \
-	 (proc)->fpLockBits &= ~(UINT64CONST(1) << FAST_PATH_BIT_POSITION(n, l))
+	 (proc)->fpLockBits[FAST_PATH_LOCK_GROUP(n)] &= ~(UINT64CONST(1) << FAST_PATH_BIT_POSITION(n, l))
 #define FAST_PATH_CHECK_LOCKMODE(proc, n, l) \
-	 ((proc)->fpLockBits & (UINT64CONST(1) << FAST_PATH_BIT_POSITION(n, l)))
+	 ((proc)->fpLockBits[FAST_PATH_LOCK_GROUP(n)] & (UINT64CONST(1) << FAST_PATH_BIT_POSITION(n, l)))
 
 /*
  * The fast-path lock mechanism is concerned only with relation locks on
@@ -926,7 +956,7 @@ LockAcquireExtended(const LOCKTAG *locktag,
 	 * for now we don't worry about that case either.
 	 */
 	if (EligibleForRelationFastPath(locktag, lockmode) &&
-		FastPathLocalUseCount < FP_LOCK_SLOTS_PER_BACKEND)
+		FastPathLocalUseCounts[FAST_PATH_LOCK_REL_GROUP(locktag->locktag_field2)] < FP_LOCK_SLOTS_PER_GROUP)
 	{
 		uint32		fasthashcode = FastPathStrongLockHashPartition(hashcode);
 		bool		acquired;
@@ -1970,6 +2000,7 @@ LockRelease(const LOCKTAG *locktag, LOCKMODE lockmode, bool sessionLock)
 	PROCLOCK   *proclock;
 	LWLock	   *partitionLock;
 	bool		wakeupNeeded;
+	int			group;
 
 	if (lockmethodid <= 0 || lockmethodid >= lengthof(LockMethods))
 		elog(ERROR, "unrecognized lock method: %d", lockmethodid);
@@ -2063,9 +2094,12 @@ LockRelease(const LOCKTAG *locktag, LOCKMODE lockmode, bool sessionLock)
 	 */
 	locallock->lockCleared = false;
 
+	/* fast-path group the lock belongs to */
+	group = FAST_PATH_LOCK_REL_GROUP(locktag->locktag_field2);
+
 	/* Attempt fast release of any lock eligible for the fast path. */
 	if (EligibleForRelationFastPath(locktag, lockmode) &&
-		FastPathLocalUseCount > 0)
+		FastPathLocalUseCounts[group] > 0)
 	{
 		bool		released;
 
@@ -2633,12 +2667,21 @@ LockReassignOwner(LOCALLOCK *locallock, ResourceOwner parent)
 static bool
 FastPathGrantRelationLock(Oid relid, LOCKMODE lockmode)
 {
-	uint32		f;
 	uint32		unused_slot = FP_LOCK_SLOTS_PER_BACKEND;
+	uint32		i,
+				group;
+
+	/* fast-path group the lock belongs to */
+	group = FAST_PATH_LOCK_REL_GROUP(relid);
 
 	/* Scan for existing entry for this relid, remembering empty slot. */
-	for (f = 0; f < FP_LOCK_SLOTS_PER_BACKEND; f++)
+	for (i = 0; i < FP_LOCK_SLOTS_PER_GROUP; i++)
 	{
+		uint32		f;
+
+		/* index into the whole per-backend array */
+		f = FP_LOCK_SLOT_INDEX(group, i);
+
 		if (FAST_PATH_GET_BITS(MyProc, f) == 0)
 			unused_slot = f;
 		else if (MyProc->fpRelId[f] == relid)
@@ -2654,7 +2697,7 @@ FastPathGrantRelationLock(Oid relid, LOCKMODE lockmode)
 	{
 		MyProc->fpRelId[unused_slot] = relid;
 		FAST_PATH_SET_LOCKMODE(MyProc, unused_slot, lockmode);
-		++FastPathLocalUseCount;
+		++FastPathLocalUseCounts[group];
 		return true;
 	}
 
@@ -2670,12 +2713,21 @@ FastPathGrantRelationLock(Oid relid, LOCKMODE lockmode)
 static bool
 FastPathUnGrantRelationLock(Oid relid, LOCKMODE lockmode)
 {
-	uint32		f;
 	bool		result = false;
+	uint32		i,
+				group;
 
-	FastPathLocalUseCount = 0;
-	for (f = 0; f < FP_LOCK_SLOTS_PER_BACKEND; f++)
+	/* fast-path group the lock belongs to */
+	group = FAST_PATH_LOCK_REL_GROUP(relid);
+
+	FastPathLocalUseCounts[group] = 0;
+	for (i = 0; i < FP_LOCK_SLOTS_PER_GROUP; i++)
 	{
+		uint32		f;
+
+		/* index into the whole per-backend array */
+		f = FP_LOCK_SLOT_INDEX(group, i);
+
 		if (MyProc->fpRelId[f] == relid
 			&& FAST_PATH_CHECK_LOCKMODE(MyProc, f, lockmode))
 		{
@@ -2685,7 +2737,7 @@ FastPathUnGrantRelationLock(Oid relid, LOCKMODE lockmode)
 			/* we continue iterating so as to update FastPathLocalUseCount */
 		}
 		if (FAST_PATH_GET_BITS(MyProc, f) != 0)
-			++FastPathLocalUseCount;
+			++FastPathLocalUseCounts[group];
 	}
 	return result;
 }
@@ -2714,7 +2766,8 @@ FastPathTransferRelationLocks(LockMethod lockMethodTable, const LOCKTAG *locktag
 	for (i = 0; i < ProcGlobal->allProcCount; i++)
 	{
 		PGPROC	   *proc = &ProcGlobal->allProcs[i];
-		uint32		f;
+		uint32		j,
+					group;
 
 		LWLockAcquire(&proc->fpInfoLock, LW_EXCLUSIVE);
 
@@ -2739,9 +2792,16 @@ FastPathTransferRelationLocks(LockMethod lockMethodTable, const LOCKTAG *locktag
 			continue;
 		}
 
-		for (f = 0; f < FP_LOCK_SLOTS_PER_BACKEND; f++)
+		/* fast-path group the lock belongs to */
+		group = FAST_PATH_LOCK_REL_GROUP(relid);
+
+		for (j = 0; j < FP_LOCK_SLOTS_PER_GROUP; j++)
 		{
 			uint32		lockmode;
+			uint32		f;
+
+			/* index into the whole per-backend array */
+			f = FP_LOCK_SLOT_INDEX(group, j);
 
 			/* Look for an allocated slot matching the given relid. */
 			if (relid != proc->fpRelId[f] || FAST_PATH_GET_BITS(proc, f) == 0)
@@ -2793,13 +2853,21 @@ FastPathGetRelationLockEntry(LOCALLOCK *locallock)
 	PROCLOCK   *proclock = NULL;
 	LWLock	   *partitionLock = LockHashPartitionLock(locallock->hashcode);
 	Oid			relid = locktag->locktag_field2;
-	uint32		f;
+	uint32		i,
+				group;
+
+	/* fast-path group the lock belongs to */
+	group = FAST_PATH_LOCK_REL_GROUP(relid);
 
 	LWLockAcquire(&MyProc->fpInfoLock, LW_EXCLUSIVE);
 
-	for (f = 0; f < FP_LOCK_SLOTS_PER_BACKEND; f++)
+	for (i = 0; i < FP_LOCK_SLOTS_PER_GROUP; i++)
 	{
 		uint32		lockmode;
+		uint32		f;
+
+		/* index into the whole per-backend array */
+		f = FP_LOCK_SLOT_INDEX(group, i);
 
 		/* Look for an allocated slot matching the given relid. */
 		if (relid != MyProc->fpRelId[f] || FAST_PATH_GET_BITS(MyProc, f) == 0)
@@ -2903,6 +2971,10 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode, int *countp)
 	LWLock	   *partitionLock;
 	int			count = 0;
 	int			fast_count = 0;
+	uint32		group;
+
+	/* fast-path group the lock belongs to */
+	group = FAST_PATH_LOCK_REL_GROUP(locktag->locktag_field2);
 
 	if (lockmethodid <= 0 || lockmethodid >= lengthof(LockMethods))
 		elog(ERROR, "unrecognized lock method: %d", lockmethodid);
@@ -2957,7 +3029,7 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode, int *countp)
 		for (i = 0; i < ProcGlobal->allProcCount; i++)
 		{
 			PGPROC	   *proc = &ProcGlobal->allProcs[i];
-			uint32		f;
+			uint32		j;
 
 			/* A backend never blocks itself */
 			if (proc == MyProc)
@@ -2979,9 +3051,13 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode, int *countp)
 				continue;
 			}
 
-			for (f = 0; f < FP_LOCK_SLOTS_PER_BACKEND; f++)
+			for (j = 0; j < FP_LOCK_SLOTS_PER_GROUP; j++)
 			{
 				uint32		lockmask;
+				uint32		f;
+
+				/* index into the whole per-backend array */
+				f = FP_LOCK_SLOT_INDEX(group, j);
 
 				/* Look for an allocated slot matching the given relid. */
 				if (relid != proc->fpRelId[f])
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index deeb06c9e01..845058da9fa 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -83,8 +83,9 @@ struct XidCache
  * rather than the main lock table.  This eases contention on the lock
  * manager LWLocks.  See storage/lmgr/README for additional details.
  */
-#define		FP_LOCK_SLOTS_PER_BACKEND 16
-
+#define		FP_LOCK_GROUPS_PER_BACKEND	64
+#define		FP_LOCK_SLOTS_PER_GROUP		16	/* don't change */
+#define		FP_LOCK_SLOTS_PER_BACKEND	(FP_LOCK_SLOTS_PER_GROUP * FP_LOCK_GROUPS_PER_BACKEND)
 /*
  * Flags for PGPROC.delayChkptFlags
  *
@@ -292,7 +293,8 @@ struct PGPROC
 
 	/* Lock manager data, recording fast-path locks taken by this backend. */
 	LWLock		fpInfoLock;		/* protects per-backend fast-path state */
-	uint64		fpLockBits;		/* lock modes held for each fast-path slot */
+	uint64		fpLockBits[FP_LOCK_GROUPS_PER_BACKEND]; /* lock modes held for
+														 * each fast-path slot */
 	Oid			fpRelId[FP_LOCK_SLOTS_PER_BACKEND]; /* slots for rel oids */
 	bool		fpVXIDLock;		/* are we holding a fast-path VXID lock? */
 	LocalTransactionId fpLocalTransactionId;	/* lxid for fast-path VXID
-- 
2.46.0

From 46c2ec00017821a32b0f9fba8e56b2ad46d9d239 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <[email-masked]>
Date: Thu, 12 Sep 2024 23:09:50 +0200
Subject: [PATCH v20240913 2/2] Set fast-path slots using
 max_locks_per_transaction

Instead of using a hard-coded value of 64 groups (1024 fast-path slots),
determine the value based on max_locks_per_transaction GUC. This size
is calculated at startup, before allocating shared memory.

The default value of max_locks_per_transaction value is 64, which means
4 groups of fast-path locks.

The purpose of the max_locks_per_transaction GUC is to size the shared
lock table, but it's the best information about the expected number of
locks available. It is often set to an average number of locks needed by
a backend, but some backends may need substantially fewer/more locks.

This means fast-path capacity calculated from max_locks_per_transaction
may not be sufficient for some backends, forcing use of the shared lock
table. The assumption is this is not a major issue - there can't be too
many of such backends, otherwise the max_locks_per_transaction would
need to be higher anyway (resolving the fast-path issue too).

If that happens to be a problem, the only solution is to increase the
GUC, even if the shared lock table had sufficient capacity. That is not
free, because each lock in the shared lock table requires about 500B.
With many backends this may be a substantial amount of memory, but then
again - that should only happen on machines with plenty of memory.

In the future we can consider a separate GUC for the number of fast-path
slots, but let's try without one first.

An alternative solution might be to size the fast-path arrays for a
multiple of max_locks_per_transaction. The cost of adding a fast-path
slot is much lower (only ~5B compared to ~500B per entry), so this would
be cheaper than increasing max_locks_per_transaction. But it's not clear
what multiple of max_locks_per_transaction to use.
---
 src/backend/bootstrap/bootstrap.c   |  2 ++
 src/backend/postmaster/postmaster.c |  5 +++
 src/backend/storage/ipc/ipci.c      |  6 ++++
 src/backend/storage/lmgr/lock.c     | 28 +++++++++++++----
 src/backend/storage/lmgr/proc.c     | 47 +++++++++++++++++++++++++++++
 src/backend/tcop/postgres.c         |  3 ++
 src/backend/utils/init/postinit.c   | 34 +++++++++++++++++++++
 src/include/miscadmin.h             |  1 +
 src/include/storage/proc.h          | 11 ++++---
 9 files changed, 126 insertions(+), 11 deletions(-)

diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index 7637581a184..ed59dfce893 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -309,6 +309,8 @@ BootstrapModeMain(int argc, char *argv[], bool check_only)
 
 	InitializeMaxBackends();
 
+	InitializeFastPathLocks();
+
 	CreateSharedMemoryAndSemaphores();
 
 	/*
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 96bc1d1cfed..f4a16595d7f 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -903,6 +903,11 @@ PostmasterMain(int argc, char *argv[])
 	 */
 	InitializeMaxBackends();
 
+	/*
+	 * Also calculate the size of the fast-path lock arrays in PGPROC.
+	 */
+	InitializeFastPathLocks();
+
 	/*
 	 * Give preloaded libraries a chance to request additional shared memory.
 	 */
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index 6caeca3a8e6..10fc18f2529 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -178,6 +178,12 @@ AttachSharedMemoryStructs(void)
 	Assert(MyProc != NULL);
 	Assert(IsUnderPostmaster);
 
+	/*
+	 * In EXEC_BACKEND mode, backends don't inherit the number of fast-path
+	 * groups we calculated before setting the shmem up, so recalculate it.
+	 */
+	InitializeFastPathLocks();
+
 	CreateOrAttachShmemStructs();
 
 	/*
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index d053ae0c409..505aa52668e 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -166,8 +166,13 @@ typedef struct TwoPhaseLockRecord
  * might be higher than the real number if another backend has transferred
  * our locks to the primary lock table, but it can never be lower than the
  * real value, since only we can acquire locks on our own behalf.
+ *
+ * XXX Allocate a static array of the maximum size. We could have a pointer
+ * and then allocate just the right size to save a couple kB, but that does
+ * not seem worth the extra complexity of having to initialize it etc. This
+ * way it gets initialized automaticaly.
  */
-static int	FastPathLocalUseCounts[FP_LOCK_GROUPS_PER_BACKEND];
+static int	FastPathLocalUseCounts[FP_LOCK_GROUPS_PER_BACKEND_MAX];
 
 /*
  * Flag to indicate if the relation extension lock is held by this backend.
@@ -184,6 +189,17 @@ static int	FastPathLocalUseCounts[FP_LOCK_GROUPS_PER_BACKEND];
  */
 static bool IsRelationExtensionLockHeld PG_USED_FOR_ASSERTS_ONLY = false;
 
+/*
+ * Number of fast-path locks per backend - size of the arrays in PGPROC.
+ * This is set only once during start, before initializing shared memory,
+ * and remains constant after that.
+ *
+ * We set the limit based on max_locks_per_transaction GUC, because that's
+ * the best information about expected number of locks per backend we have.
+ * See InitializeFastPathLocks for details.
+ */
+int			FastPathLockGroupsPerBackend = 0;
+
 /*
  * Macros to calculate the group and index for a relation.
  *
@@ -195,11 +211,11 @@ static bool IsRelationExtensionLockHeld PG_USED_FOR_ASSERTS_ONLY = false;
  * small enough to not cause overflows (in 64-bit).
  */
 #define FAST_PATH_LOCK_REL_GROUP(rel) \
-	(((uint64) (rel) * 49157) % FP_LOCK_GROUPS_PER_BACKEND)
+	(((uint64) (rel) * 49157) % FastPathLockGroupsPerBackend)
 
 /* Calculate index in the whole per-backend array of lock slots. */
 #define FP_LOCK_SLOT_INDEX(group, index) \
-	(AssertMacro(((group) >= 0) && ((group) < FP_LOCK_GROUPS_PER_BACKEND)), \
+	(AssertMacro(((group) >= 0) && ((group) < FastPathLockGroupsPerBackend)), \
 	 AssertMacro(((index) >= 0) && ((index) < FP_LOCK_SLOTS_PER_GROUP)), \
 	 ((group) * FP_LOCK_SLOTS_PER_GROUP + (index)))
 
@@ -2973,9 +2989,6 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode, int *countp)
 	int			fast_count = 0;
 	uint32		group;
 
-	/* fast-path group the lock belongs to */
-	group = FAST_PATH_LOCK_REL_GROUP(locktag->locktag_field2);
-
 	if (lockmethodid <= 0 || lockmethodid >= lengthof(LockMethods))
 		elog(ERROR, "unrecognized lock method: %d", lockmethodid);
 	lockMethodTable = LockMethods[lockmethodid];
@@ -3005,6 +3018,9 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode, int *countp)
 	partitionLock = LockHashPartitionLock(hashcode);
 	conflictMask = lockMethodTable->conflictTab[lockmode];
 
+	/* fast-path group the lock belongs to */
+	group = FAST_PATH_LOCK_REL_GROUP(locktag->locktag_field2);
+
 	/*
 	 * Fast path locks might not have been entered in the primary lock table.
 	 * If the lock we're dealing with could conflict with such a lock, we must
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index ac66da8638f..a91b6f8a6c0 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -103,6 +103,8 @@ ProcGlobalShmemSize(void)
 	Size		size = 0;
 	Size		TotalProcs =
 		add_size(MaxBackends, add_size(NUM_AUXILIARY_PROCS, max_prepared_xacts));
+	Size		fpLockBitsSize,
+				fpRelIdSize;
 
 	/* ProcGlobal */
 	size = add_size(size, sizeof(PROC_HDR));
@@ -113,6 +115,18 @@ ProcGlobalShmemSize(void)
 	size = add_size(size, mul_size(TotalProcs, sizeof(*ProcGlobal->subxidStates)));
 	size = add_size(size, mul_size(TotalProcs, sizeof(*ProcGlobal->statusFlags)));
 
+	/*
+	 * fast-path lock arrays
+	 *
+	 * XXX The explicit alignment may not be strictly necessary, as both
+	 * values are already multiples of 8 bytes, which is what MAXALIGN does.
+	 * But better to make that obvious.
+	 */
+	fpLockBitsSize = MAXALIGN(FastPathLockGroupsPerBackend * sizeof(uint64));
+	fpRelIdSize = MAXALIGN(FastPathLockGroupsPerBackend * sizeof(Oid) * FP_LOCK_SLOTS_PER_GROUP);
+
+	size = add_size(size, mul_size(TotalProcs, (fpLockBitsSize + fpRelIdSize)));
+
 	return size;
 }
 
@@ -162,6 +176,10 @@ InitProcGlobal(void)
 				j;
 	bool		found;
 	uint32		TotalProcs = MaxBackends + NUM_AUXILIARY_PROCS + max_prepared_xacts;
+	char	   *fpPtr,
+			   *fpEndPtr PG_USED_FOR_ASSERTS_ONLY;
+	Size		fpLockBitsSize,
+				fpRelIdSize;
 
 	/* Create the ProcGlobal shared structure */
 	ProcGlobal = (PROC_HDR *)
@@ -211,12 +229,38 @@ InitProcGlobal(void)
 	ProcGlobal->statusFlags = (uint8 *) ShmemAlloc(TotalProcs * sizeof(*ProcGlobal->statusFlags));
 	MemSet(ProcGlobal->statusFlags, 0, TotalProcs * sizeof(*ProcGlobal->statusFlags));
 
+	/*
+	 * Allocate arrays for fast-path locks. Those are variable-length, so
+	 * can't be included in PGPROC. We allocate a separate piece of shared
+	 * memory and then divide that between backends.
+	 */
+	fpLockBitsSize = MAXALIGN(FastPathLockGroupsPerBackend * sizeof(uint64));
+	fpRelIdSize = MAXALIGN(FastPathLockGroupsPerBackend * sizeof(Oid) * FP_LOCK_SLOTS_PER_GROUP);
+
+	fpPtr = ShmemAlloc(TotalProcs * (fpLockBitsSize + fpRelIdSize));
+	MemSet(fpPtr, 0, TotalProcs * (fpLockBitsSize + fpRelIdSize));
+
+	/* For asserts checking we did not overflow. */
+	fpEndPtr = fpPtr + (TotalProcs * (fpLockBitsSize + fpRelIdSize));
+
 	for (i = 0; i < TotalProcs; i++)
 	{
 		PGPROC	   *proc = &procs[i];
 
 		/* Common initialization for all PGPROCs, regardless of type. */
 
+		/*
+		 * Set the fast-path lock arrays, and move the pointer. We interleave
+		 * the two arrays, to keep at least some locality.
+		 */
+		proc->fpLockBits = (uint64 *) fpPtr;
+		fpPtr += fpLockBitsSize;
+
+		proc->fpRelId = (Oid *) fpPtr;
+		fpPtr += fpRelIdSize;
+
+		Assert(fpPtr <= fpEndPtr);
+
 		/*
 		 * Set up per-PGPROC semaphore, latch, and fpInfoLock.  Prepared xact
 		 * dummy PGPROCs don't need these though - they're never associated
@@ -278,6 +322,9 @@ InitProcGlobal(void)
 		pg_atomic_init_u64(&(proc->waitStart), 0);
 	}
 
+	/* We expect to consume exactly the expected amount of data. */
+	Assert(fpPtr = fpEndPtr);
+
 	/*
 	 * Save pointers to the blocks of PGPROC structures reserved for auxiliary
 	 * processes and prepared transactions.
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 8bc6bea1135..f54ae00abca 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -4166,6 +4166,9 @@ PostgresSingleUserMain(int argc, char *argv[],
 	/* Initialize MaxBackends */
 	InitializeMaxBackends();
 
+	/* Initialize size of fast-path lock cache. */
+	InitializeFastPathLocks();
+
 	/*
 	 * Give preloaded libraries a chance to request additional shared memory.
 	 */
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 3b50ce19a2c..1faf756c8d8 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -557,6 +557,40 @@ InitializeMaxBackends(void)
 						   MAX_BACKENDS)));
 }
 
+/*
+ * Initialize the number of fast-path lock slots in PGPROC.
+ *
+ * This must be called after modules have had the chance to alter GUCs in
+ * shared_preload_libraries and before shared memory size is determined.
+ *
+ * The default max_locks_per_xact=64 means 4 groups by default.
+ *
+ * We allow anything between 1 and 1024 groups, with the usual power-of-2
+ * logic. The 1 is the "old" value before allowing multiple groups, 1024
+ * is an arbitrary limit (matching max_locks_per_xact = 16k). Values over
+ * 1024 are unlikely to be beneficial - we're likely to hit other
+ * bottlenecks long before that.
+ */
+void
+InitializeFastPathLocks(void)
+{
+	Assert(FastPathLockGroupsPerBackend == 0);
+
+	/* we need at least one group */
+	FastPathLockGroupsPerBackend = 1;
+
+	while (FastPathLockGroupsPerBackend < FP_LOCK_GROUPS_PER_BACKEND_MAX)
+	{
+		/* stop once we exceed max_locks_per_xact */
+		if (FastPathLockGroupsPerBackend * FP_LOCK_SLOTS_PER_GROUP >= max_locks_per_xact)
+			break;
+
+		FastPathLockGroupsPerBackend *= 2;
+	}
+
+	Assert(FastPathLockGroupsPerBackend <= FP_LOCK_GROUPS_PER_BACKEND_MAX);
+}
+
 /*
  * Early initialization of a backend (either standalone or under postmaster).
  * This happens even before InitPostgres.
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 25348e71eb9..e26d108a470 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -475,6 +475,7 @@ extern PGDLLIMPORT ProcessingMode Mode;
 #define INIT_PG_OVERRIDE_ROLE_LOGIN		0x0004
 extern void pg_split_opts(char **argv, int *argcp, const char *optstr);
 extern void InitializeMaxBackends(void);
+extern void InitializeFastPathLocks(void);
 extern void InitPostgres(const char *in_dbname, Oid dboid,
 						 const char *username, Oid useroid,
 						 bits32 flags,
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 845058da9fa..0e55c166529 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -83,9 +83,11 @@ struct XidCache
  * rather than the main lock table.  This eases contention on the lock
  * manager LWLocks.  See storage/lmgr/README for additional details.
  */
-#define		FP_LOCK_GROUPS_PER_BACKEND	64
+extern PGDLLIMPORT int FastPathLockGroupsPerBackend;
+#define		FP_LOCK_GROUPS_PER_BACKEND_MAX	1024
 #define		FP_LOCK_SLOTS_PER_GROUP		16	/* don't change */
-#define		FP_LOCK_SLOTS_PER_BACKEND	(FP_LOCK_SLOTS_PER_GROUP * FP_LOCK_GROUPS_PER_BACKEND)
+#define		FP_LOCK_SLOTS_PER_BACKEND	(FP_LOCK_SLOTS_PER_GROUP * FastPathLockGroupsPerBackend)
+
 /*
  * Flags for PGPROC.delayChkptFlags
  *
@@ -293,9 +295,8 @@ struct PGPROC
 
 	/* Lock manager data, recording fast-path locks taken by this backend. */
 	LWLock		fpInfoLock;		/* protects per-backend fast-path state */
-	uint64		fpLockBits[FP_LOCK_GROUPS_PER_BACKEND]; /* lock modes held for
-														 * each fast-path slot */
-	Oid			fpRelId[FP_LOCK_SLOTS_PER_BACKEND]; /* slots for rel oids */
+	uint64	   *fpLockBits;		/* lock modes held for each fast-path slot */
+	Oid		   *fpRelId;		/* slots for rel oids */
 	bool		fpVXIDLock;		/* are we holding a fast-path VXID lock? */
 	LocalTransactionId fpLocalTransactionId;	/* lxid for fast-path VXID
 												 * lock */
-- 
2.46.0

---

From: Jakub Wartak <[email-masked]>
Date: Mon, 16 Sep 2024 15:11:31 +0200

On Fri, Sep 13, 2024 at 1:45 AM Tomas Vondra <[email-masked]> wrote:

> [..]

> Anyway, at this point I'm quite happy with this improvement. I didn't
> have any clear plan when to commit this, but I'm considering doing so
> sometime next week, unless someone objects or asks for some additional
> benchmarks etc.

Thank you very much for working on this :)

The only fact that comes to my mind is that we could blow up L2
caches. Fun fact, so if we are growing PGPROC by 6.3x, that's going to
be like one or two 2MB huge pages more @ common max_connections=1000
x86_64 (830kB -> ~5.1MB), and indeed:

# without patch:
postgres@hive:~$ /usr/pgsql18/bin/postgres -D /tmp/pg18 -C
shared_memory_size_in_huge_pages
177

# with patch:
postgres@hive:~$ /usr/pgsql18/bin/postgres -D /tmp/pg18 -C
shared_memory_size_in_huge_pages
178

So playing Devil's advocate , the worst situation that could possibly
hurt (?) could be:
* memory size of PGPROC working set >> L2_cache (thus very high
max_connections),
* insane number of working sessions on CPU (sessions >> VCPU) - sadly
happens to some,
* those sessions wouldn't have to be competing for the same Oids -
just fetching this new big fpLockBits[] structure - so probing a lot
for lots of Oids, but *NOT* having to use futex() syscall [so not that
syscall price]
* no huge pages (to cause dTLB misses)

then maybe(?) one could observe further degradation of dTLB misses in
the perf-stat counter under some microbenchmark, but measuring that
requires isolated and physical hardware. Maybe that would be actually
noise due to overhead of context-switches itself. Just trying to think
out loud, what big PGPROC could cause here. But this is already an
unhealthy and non-steady state of the system, so IMHO we are good,
unless someone comes up with a better (more evil) idea.

>> I did look at docs if anything needs updating, but I don't think so. The
SGML docs only talk about fast-path locking at fairly high level, not
about how many we have etc.

Well the only thing I could think of was to add to the
doc/src/sgml/config.sgml / "max_locks_per_transaction" GUC, that "it
is also used as advisory for the number of groups used in
lockmanager's fast-path implementation" (that is, without going into
further discussion, as even pg_locks discussion
doc/src/sgml/system-views.sgml simply uses that term).

-J.

---

From: Tomas Vondra <[email-masked]>
Date: Mon, 16 Sep 2024 16:19:29 +0200

> unless someone comes up with a better (more evil) idea.

I've been thinking about such cases too, but I don't think it can really
happen in practice, because:

- How likely is it that the sessions will need a lot of OIDs, but not
the same ones? Also, why would it matter that the OIDs are not the same,
I don't think it matters unless one of the sessions needs an exclusive
lock, at which point the optimization doesn't really matter.

- If having more fast-path slots means it doesn't fit into L2 cache,
would we fit into L2 without it? I don't think so - if there really are
that many locks, we'd have to add those into the shared lock table, and
there's a lot of extra stuff to keep in memory (relcaches, ...).

This is pretty much one of the cases I focused on in my benchmarking,
and I'm yet to see any regression.

> doc/src/sgml/system-views.sgml simply uses that term).

Thanks, I'll consider mentioning this in max_locks_per_transaction.
Also, I think there's a place calculating the amount of per-connection
memory, so maybe that needs to be updated too.

regards

-- 
Tomas Vondra

---

From: Tomas Vondra <[email-masked]>
Date: Tue, 17 Sep 2024 22:16:04 +0200

I've spent the last couple days doing all kinds of experiments trying to
find regressions caused by the patch, but no success. Which is good.

Attached is a script that just does a simple pgbench on a tiny table,
with no or very few partitions. The idea is that this will will fit into
shared buffers (thus no I/O), and will fit into the 16 fast-path slots
we have now. It can't benefit from the patch - it can only get worse, if
having more fast-path slots hurts.

I ran this on my two machines, and in both cases the results are +/- 1%
from the master for all combinations of parameters (clients, mode,
number of partitions, ..). In most cases it's actually much closer,
particularly with the default max_locks_per_transaction value.

For higher values of the GUC, I think it's fine too - the differences
are perhaps a bit larger (~1.5%), but it's clearly hardware specific (i5
gets a bit faster, xeon a bit slower). And I'm pretty sure people who
increased that GUC value likely did that because of locking many rels,
and so will actually benefit from the increased fast-path capacity.

At this point I'm pretty happy and confident the patch is fine. Unless
someone objects, I'll get it committed after going over over it one more
time. I decided to commit that as as a single change - it would be weird
to have an intermediate state with larger arrays in PGPROC, when that's
not something we actually want.

I still haven't found any places in the docs that should mention this,
except for the bit about max_locks_per_transaction GUC. There's nothing
in SGML mentioning details of fast-path locking. I thought we have some
formula to calculate per-connection memory, but I think I confused that
with the shmmem formulas we had in "Managing Kernel Resources". But even
that no longer mentions max_connections in master.

regards

-- 
Tomas Vondra

---

From: Tomas Vondra <[email-masked]>
Date: Sat, 21 Sep 2024 20:33:49 +0200

Hi,

I've finally pushed this, after many rounds of careful testing to ensure
no regressions, and polishing. All changes since the version shared on
September 13 are only cosmetic - renaming a macro to keep it consistent
with the other ones, clarifying a couple comments etc. Nothing major.

I ended up squashing the two parts into a single commit. I thought about
keeping the two steps, but it seemed pointless - the first part inflated
the PGPROC struct, which I didn't like to commit, even if only as an
intermediate WIP state.

So far buildfarm didn't blew up, so let's hope it will stay that way.

I just realized there's no CF entry for this - sorry about that :-( I
started the thread a year ago to discuss an experimental patche, and it
never made it to CFA. But there was a discussion spanning a year, so
hopefully that's enough.

regards

-- 
Tomas Vondra

---

From: Ants Aasma <[email-masked]>
Date: Sun, 22 Sep 2024 11:50:31 +0300

> with the other ones, clarifying a couple comments etc. Nothing major.

Great work on this, I have seen multiple customers hitting fast path
capacity related LockManager contention. They will certainly be glad
to have a fix available when they eventually upgrade. Regretfully I
did not find the time to participate in this discussion during
development. But I did have some thoughts that I wanted to unload to
the list, not as a criticism, but in case it turns out follow up work
is needed.

Driving the array sizing from max_locks_per_transaction seems like a
good idea. The one major difference from the lock table is that while
the lock table is partitioned dynamically between backends, the fast
path array has a static size per backend. One case where that
distinction matters is when only a fraction of backends try to lock
large numbers of relations. This fraction will still fall back to main
lock tables, but at least the contention should be limited by virtue
of not having too many of those backends. The other case is when
max_connections is much higher than the number of backends actually
used. Then backends may be consuming well over
max_locks_per_transaction without running into lock table capacity
issues.

In both cases users will have the simple workaround of just increasing
the max_locks_per_transaction setting.  Still, I'm sure they would be
happier if things just worked without any tuning. So I tried to figure
out some scheme to get dynamic allocation of fast path locks.

The best data structure I came up with was to have a shared fast path
lock array. Still partitioned as a 16-way associative cache, but
indexed by hash(BackendId, RelationId). fpLockBits can be stuffed into
the high byte of BackendId thanks to MAX_BACKENDS. Locking could be
handled by one lock per way, or at least on cursory glance it
shouldn't be too difficult to convert the whole fast path acquisition
to be lock free.

Either way, it feels like structuring the array this way could result
in a large amount of false sharing of cache lines. Current static
allocation means that each process needs to touch only a small set of
cache lines only referenced by itself - quite probable to keep those
lines in CPU local L2 in exclusive mode. In a shared array a larger
number of cache lines are needed and they will be concurrently written
to by other backends - lots of invalidation messages and cache line
bouncing. I don't know how large this effect will be without doing a
prototype and running it on a large machine with high core-to-core
latencies.

It would be possible to create a hybrid approach of a small local FP
array servicing the majority of acquisitions with a larger shared
victim cache for exceptional cases. But it doesn't feel like it is
worth the complexity. At least not without seeing some example
workloads where it would help. And even then, maybe using hierarchical
locking to do less work is the better approach.

Being optimistic, perhaps the current patch was enough to resolve the issue.

--
Ants Aasma
Senior Database Engineer
www.cybertec-postgresql.com

---

From: Tomas Vondra <[email-masked]>
Date: Sun, 22 Sep 2024 14:14:21 +0200

> max_locks_per_transaction without running into lock table capacity

I agree. I don't think the case with a couple lock-hungry backends
matters too much, because as you say there can't be too many of them, so
the contention should not be too bad. At least that was my reasoning.

Regarding the case with very high max_connection values - I doubt we
want to optimize for that very much. Extremely high max_connection
values are a clear anti-pattern (IMO), and if you choose to do that
anyway, you simply have to accept that connections have costs. The
memory for fast-path locking is one of those costs.

I'm not against improving that, ofc, but I think we should only do that
if it doesn't hurt the "reasonable" setups.

> out some scheme to get dynamic allocation of fast path locks.

I agree with the premise that less tuning is better. Which is why we
tied this to max_locks_per_transaction.

> prototype and running it on a large machine with high core-to-core

I don't have a very good intuition regarding cachelines. Ideally, the
backends would access disjunct parts of the array, so there should not
be a lot of false sharing. But maybe I'm wrong, hard to say without an
experimental patch.

> locking to do less work is the better approach.

Not sure. My intuition would be to keep this as simple a possible.
Having a shared lock table and also a separate fast-path cache is
already sufficiently complex, adding cache for a cache seems a bit too
much to me.

> Being optimistic, perhaps the current patch was enough to resolve the issue.

It's an improvement. But if you want to give the shared fast-path cache
a try, go ahead - if you write a patch, I promise to review it.

regards

-- 
Tomas Vondra

---

From: Tom Lane <[email-masked]>
Date: Sun, 22 Sep 2024 11:45:59 -0400

Tomas Vondra <[email-masked]> writes:
> no regressions, and polishing.

Coverity is not terribly happy with this.  "Assert(fpPtr = fpEndPtr);"
is very clearly not doing what you presumably intended.  The others
look like overaggressive assertion checking.  If you don't want those
macros to assume that the argument is unsigned, you could force the
issue, say with

 #define FAST_PATH_GROUP(index)	\
-	(AssertMacro(((index) >= 0) && ((index) < FP_LOCK_SLOTS_PER_BACKEND)), \
+	(AssertMacro((uint32) (index) < FP_LOCK_SLOTS_PER_BACKEND), \
 	 ((index) / FP_LOCK_SLOTS_PER_GROUP))

________________________________________________________________________________________________________
*** CID 1619664:  Incorrect expression  (ASSERT_SIDE_EFFECT)
/srv/coverity/git/pgsql-git/postgresql/src/backend/storage/lmgr/proc.c: 325 in InitProcGlobal()
319     		pg_atomic_init_u32(&(proc->procArrayGroupNext), INVALID_PROC_NUMBER);
320     		pg_atomic_init_u32(&(proc->clogGroupNext), INVALID_PROC_NUMBER);
321     		pg_atomic_init_u64(&(proc->waitStart), 0);
322     	}
323     
324     	/* Should have consumed exactly the expected amount of fast-path memory. */
>>>     CID 1619664:  Incorrect expression  (ASSERT_SIDE_EFFECT)
>>>     Assignment "fpPtr = fpEndPtr" has a side effect.  This code will work differently in a non-debug build.
325     	Assert(fpPtr = fpEndPtr);
326     
327     	/*
328     	 * Save pointers to the blocks of PGPROC structures reserved for auxiliary
329     	 * processes and prepared transactions.
330     	 */

________________________________________________________________________________________________________
*** CID 1619662:  Integer handling issues  (NO_EFFECT)
/srv/coverity/git/pgsql-git/postgresql/src/backend/storage/lmgr/lock.c: 3731 in GetLockStatusData()
3725     
3726     		LWLockAcquire(&proc->fpInfoLock, LW_SHARED);
3727     
3728     		for (f = 0; f < FP_LOCK_SLOTS_PER_BACKEND; ++f)
3729     		{
3730     			LockInstanceData *instance;
>>>     CID 1619662:  Integer handling issues  (NO_EFFECT)
>>>     This greater-than-or-equal-to-zero comparison of an unsigned value is always true. "f >= 0U".
3731     			uint32		lockbits = FAST_PATH_GET_BITS(proc, f);
3732     
3733     			/* Skip unallocated slots. */
3734     			if (!lockbits)
3735     				continue;
3736     

________________________________________________________________________________________________________
*** CID 1619661:  Integer handling issues  (NO_EFFECT)
/srv/coverity/git/pgsql-git/postgresql/src/backend/storage/lmgr/lock.c: 2696 in FastPathGrantRelationLock()
2690     	uint32		group = FAST_PATH_REL_GROUP(relid);
2691     
2692     	/* Scan for existing entry for this relid, remembering empty slot. */
2693     	for (i = 0; i < FP_LOCK_SLOTS_PER_GROUP; i++)
2694     	{
2695     		/* index into the whole per-backend array */
>>>     CID 1619661:  Integer handling issues  (NO_EFFECT)
>>>     This greater-than-or-equal-to-zero comparison of an unsigned value is always true. "group >= 0U".
2696     		uint32		f = FAST_PATH_SLOT(group, i);
2697     
2698     		if (FAST_PATH_GET_BITS(MyProc, f) == 0)
2699     			unused_slot = f;
2700     		else if (MyProc->fpRelId[f] == relid)
2701     		{

________________________________________________________________________________________________________
*** CID 1619660:  Integer handling issues  (NO_EFFECT)
/srv/coverity/git/pgsql-git/postgresql/src/backend/storage/lmgr/lock.c: 2813 in FastPathTransferRelationLocks()
2807     
2808     		for (j = 0; j < FP_LOCK_SLOTS_PER_GROUP; j++)
2809     		{
2810     			uint32		lockmode;
2811     
2812     			/* index into the whole per-backend array */
>>>     CID 1619660:  Integer handling issues  (NO_EFFECT)
>>>     This greater-than-or-equal-to-zero comparison of an unsigned value is always true. "group >= 0U".
2813     			uint32		f = FAST_PATH_SLOT(group, j);
2814     
2815     			/* Look for an allocated slot matching the given relid. */
2816     			if (relid != proc->fpRelId[f] || FAST_PATH_GET_BITS(proc, f) == 0)
2817     				continue;
2818     

________________________________________________________________________________________________________
*** CID 1619659:  Integer handling issues  (NO_EFFECT)
/srv/coverity/git/pgsql-git/postgresql/src/backend/storage/lmgr/lock.c: 3067 in GetLockConflicts()
3061     
3062     			for (j = 0; j < FP_LOCK_SLOTS_PER_GROUP; j++)
3063     			{
3064     				uint32		lockmask;
3065     
3066     				/* index into the whole per-backend array */
>>>     CID 1619659:  Integer handling issues  (NO_EFFECT)
>>>     This greater-than-or-equal-to-zero comparison of an unsigned value is always true. "group >= 0U".
3067     				uint32		f = FAST_PATH_SLOT(group, j);
3068     
3069     				/* Look for an allocated slot matching the given relid. */
3070     				if (relid != proc->fpRelId[f])
3071     					continue;
3072     				lockmask = FAST_PATH_GET_BITS(proc, f);

________________________________________________________________________________________________________
*** CID 1619658:  Integer handling issues  (NO_EFFECT)
/srv/coverity/git/pgsql-git/postgresql/src/backend/storage/lmgr/lock.c: 2739 in FastPathUnGrantRelationLock()
2733     	uint32		group = FAST_PATH_REL_GROUP(relid);
2734     
2735     	FastPathLocalUseCounts[group] = 0;
2736     	for (i = 0; i < FP_LOCK_SLOTS_PER_GROUP; i++)
2737     	{
2738     		/* index into the whole per-backend array */
>>>     CID 1619658:  Integer handling issues  (NO_EFFECT)
>>>     This greater-than-or-equal-to-zero comparison of an unsigned value is always true. "group >= 0U".
2739     		uint32		f = FAST_PATH_SLOT(group, i);
2740     
2741     		if (MyProc->fpRelId[f] == relid
2742     			&& FAST_PATH_CHECK_LOCKMODE(MyProc, f, lockmode))
2743     		{
2744     			Assert(!result);

________________________________________________________________________________________________________
*** CID 1619657:  Integer handling issues  (NO_EFFECT)
/srv/coverity/git/pgsql-git/postgresql/src/backend/storage/lmgr/lock.c: 2878 in FastPathGetRelationLockEntry()
2872     
2873     	for (i = 0; i < FP_LOCK_SLOTS_PER_GROUP; i++)
2874     	{
2875     		uint32		lockmode;
2876     
2877     		/* index into the whole per-backend array */
>>>     CID 1619657:  Integer handling issues  (NO_EFFECT)
>>>     This greater-than-or-equal-to-zero comparison of an unsigned value is always true. "group >= 0U".
2878     		uint32		f = FAST_PATH_SLOT(group, i);
2879     
2880     		/* Look for an allocated slot matching the given relid. */
2881     		if (relid != MyProc->fpRelId[f] || FAST_PATH_GET_BITS(MyProc, f) == 0)
2882     			continue;
2883     

			regards, tom lane

---

From: Tomas Vondra <[email-masked]>
Date: Mon, 23 Sep 2024 00:50:41 +0200

>  	 ((index) / FP_LOCK_SLOTS_PER_GROUP))

Ah, you're right. I'll fix those asserts tomorrow.

The first is clearly wrong, of course.

For the (x >= 0) asserts, doing it this way relies on negative values
wrapping to large positive ones, correct? AFAIK it's guaranteed to be a
very large value, so it can't accidentally be less than the slot count.

regards

-- 
Tomas Vondra

---

From: Tom Lane <[email-masked]>
Date: Sun, 22 Sep 2024 19:06:46 -0400

> very large value, so it can't accidentally be less than the slot count.

Right, any negative value would wrap to something more than
INT32_MAX.

			regards, tom lane

---

From: Jakub Wartak <[email-masked]>
Date: Mon, 23 Sep 2024 12:02:24 +0200

[..]
> and I'm yet to see any regression.

Sorry for answering this so late. Just for context here: I was
imagining a scenario with high max_connections about e.g. schema-based
multi-tenancy and no partitioning (so all would be fine without this
$thread/commit ; so under 16 (fast)locks would be taken). The OIDs
need to be different to avoid contention: so that futex() does not end
up really in syscall (just user-space part). My theory was that a much
smaller PGPROC should be doing much less (data) cache-line fetches
than with-the-patch. That hash() % prime , hits various parts of a
larger array - so without patch should be quicker as it wouldn't be
randomly hitting some larger array[], but it might be noise as you
state.  It was a theoretical attempt at crafting the worst possible
conditions for the patch, so feel free to disregard as it already
assumes some anti-pattern (big & all active max_connections).

> memory, so maybe that needs to be updated too.

I couldn't find it in current versions, but maybe that's helpful/reaffirming:
- up to 9.2. there were exact formulas used, see "(1800 + 270 *
max_locks_per_transaction) * max_connections" [1] , that's a long time
gone now.
- if anything then Andres might want to improve a little his blog
entry: [1] (my take is that is seems to be the most accurate and
authoritative technical information that we have online)

-J.

[1] - https://www.postgresql.org/docs/9.2/kernel-resources.html
[2] - https://blog.anarazel.de/2020/10/07/measuring-the-memory-overhead-of-a-postgres-connection/

---

From: Tomas Vondra <[email-masked]>
Date: Mon, 23 Sep 2024 12:26:40 +0200

> Right, any negative value would wrap to something more than

Thanks. Pushed a fix for these issues, hopefully coverity will be happy.

BTW is the coverity report accessible somewhere? I know someone
mentioned that in the past, but I don't recall the details. Maybe we
should have a list of all these resources, useful for committers,
somewhere on the wiki?

regards

-- 
Tomas Vondra

---

From: Tom Lane <[email-masked]>
Date: Mon, 23 Sep 2024 10:37:16 -0400

> Thanks. Pushed a fix for these issues, hopefully coverity will be happy.

Thanks.

> should have a list of all these resources, useful for committers,

Currently those reports only go to the security team.  Perhaps
we should rethink that?

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Mon, 3 Mar 2025 13:10:08 -0500

Hi,

> no regressions, and polishing.

One minor nit: I don't like that FP_LOCK_SLOTS_PER_BACKEND is now non-constant
while looking like a constant:

#define		FP_LOCK_SLOTS_PER_BACKEND	(FP_LOCK_SLOTS_PER_GROUP * FastPathLockGroupsPerBackend)

I don't think it's a good idea to have non-function-like #defines that
reference variables that can change from run to run.

Greetings,

Andres Freund

---

From: Tomas Vondra <[email-masked]>
Date: Mon, 3 Mar 2025 21:31:42 +0100

> reference variables that can change from run to run.

Fair point, although it can't change "run to run" - not without a
restart. It's not a proper constant, of course, but it seemed close
enough. Yes, it might confuse people into thinking it's a constant, or
is there some additional impact?

The one fix I can think of is making it look more like a function,
possibly just like this:

#define	FastPathLockSlotsPerBackend() \
  (FP_LOCK_SLOTS_PER_GROUP * FastPathLockGroupsPerBackend)

Or do you have another suggestion?

regards

-- 
Tomas Vondra

---

From: Andres Freund <[email-masked]>
Date: Mon, 3 Mar 2025 15:52:09 -0500

Hi,

On 2025-03-03 21:31:42 +0100, Tomas Vondra wrote:
> restart.

That's what I meant with "run to run".

> It's not a proper constant, of course, but it seemed close

That seems plenty. I just looked at the shem sizing function and was confused
because I didn't see where the max_locks_per_transaction affects the
allocation size.

> Or do you have another suggestion?

That'd work for me.

Greetings,

Andres Freund

---

From: Tomas Vondra <[email-masked]>
Date: Tue, 4 Mar 2025 14:05:22 +0100

> That's what I meant with "run to run".

OK.

> because I didn't see where the max_locks_per_transaction affects the

But the shmem sizing doesn't use FP_LOCK_SLOTS_PER_BACKEND at all, both
proc.c and postinit.c use the "full" formula, not the macro

  FastPathLockGroupsPerBackend * FP_LOCK_SLOTS_PER_GROUP

so why would the macro make this bit less obvious?

>> Or do you have another suggestion?

Attached is a patch doing this, but considering it has nothing to do
with the shmem sizing, I wonder if it's worth it.

regards

-- 
Tomas Vondra
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index 11b4d1085bb..ccfe6b69bf5 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -226,10 +226,10 @@ int			FastPathLockGroupsPerBackend = 0;
  * the FAST_PATH_SLOT macro, split it into group and index (in the group).
  */
 #define FAST_PATH_GROUP(index)	\
-	(AssertMacro((uint32) (index) < FP_LOCK_SLOTS_PER_BACKEND), \
+	(AssertMacro((uint32) (index) < FastPathLockSlotsPerBackend()), \
 	 ((index) / FP_LOCK_SLOTS_PER_GROUP))
 #define FAST_PATH_INDEX(index)	\
-	(AssertMacro((uint32) (index) < FP_LOCK_SLOTS_PER_BACKEND), \
+	(AssertMacro((uint32) (index) < FastPathLockSlotsPerBackend()), \
 	 ((index) % FP_LOCK_SLOTS_PER_GROUP))
 
 /* Macros for manipulating proc->fpLockBits */
@@ -242,7 +242,7 @@ int			FastPathLockGroupsPerBackend = 0;
 #define FAST_PATH_BIT_POSITION(n, l) \
 	(AssertMacro((l) >= FAST_PATH_LOCKNUMBER_OFFSET), \
 	 AssertMacro((l) < FAST_PATH_BITS_PER_SLOT+FAST_PATH_LOCKNUMBER_OFFSET), \
-	 AssertMacro((n) < FP_LOCK_SLOTS_PER_BACKEND), \
+	 AssertMacro((n) < FastPathLockSlotsPerBackend()), \
 	 ((l) - FAST_PATH_LOCKNUMBER_OFFSET + FAST_PATH_BITS_PER_SLOT * (FAST_PATH_INDEX(n))))
 #define FAST_PATH_SET_LOCKMODE(proc, n, l) \
 	 FAST_PATH_BITS(proc, n) |= UINT64CONST(1) << FAST_PATH_BIT_POSITION(n, l)
@@ -2691,7 +2691,7 @@ static bool
 FastPathGrantRelationLock(Oid relid, LOCKMODE lockmode)
 {
 	uint32		i;
-	uint32		unused_slot = FP_LOCK_SLOTS_PER_BACKEND;
+	uint32		unused_slot = FastPathLockSlotsPerBackend();
 
 	/* fast-path group the lock belongs to */
 	uint32		group = FAST_PATH_REL_GROUP(relid);
@@ -2713,7 +2713,7 @@ FastPathGrantRelationLock(Oid relid, LOCKMODE lockmode)
 	}
 
 	/* If no existing entry, use any empty slot. */
-	if (unused_slot < FP_LOCK_SLOTS_PER_BACKEND)
+	if (unused_slot < FastPathLockSlotsPerBackend())
 	{
 		MyProc->fpRelId[unused_slot] = relid;
 		FAST_PATH_SET_LOCKMODE(MyProc, unused_slot, lockmode);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 20777f7d5ae..114eb1f8f76 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -88,7 +88,8 @@ extern PGDLLIMPORT int FastPathLockGroupsPerBackend;
 
 #define		FP_LOCK_GROUPS_PER_BACKEND_MAX	1024
 #define		FP_LOCK_SLOTS_PER_GROUP		16	/* don't change */
-#define		FP_LOCK_SLOTS_PER_BACKEND	(FP_LOCK_SLOTS_PER_GROUP * FastPathLockGroupsPerBackend)
+#define		FastPathLockSlotsPerBackend() \
+	(FP_LOCK_SLOTS_PER_GROUP * FastPathLockGroupsPerBackend)
 
 /*

---

From: Andres Freund <[email-masked]>
Date: Tue, 4 Mar 2025 08:11:03 -0500

Hi,

> proc.c and postinit.c use the "full" formula, not the macro

Not sure what I brainfarted there...

> with the shmem sizing, I wonder if it's worth it.

Yes.

Greetings,

Andres Freund

---

From: Tomas Vondra <[email-masked]>
Date: Tue, 4 Mar 2025 15:38:17 +0100

> Not sure what I brainfarted there...

This got me thinking - maybe it'd be better to use the new
FastPathLockSlotsPerBackend() in all places that need the number of
slots per backend, including those in proc.c etc.? Arguably, these
places should have used FP_LOCK_SLOTS_PER_BACKEND before.

The attached v2 patch does that.

>> with the shmem sizing, I wonder if it's worth it.

OK, barring objections I'll push the v2.

regards

 
 
 	/* If no existing entry, use any empty slot. */
-	if (unused_slot < FP_LOCK_SLOTS_PER_BACKEND)
+	if (unused_slot < FastPathLockSlotsPerBackend())
 	{
 		MyProc->fpRelId[unused_slot] = relid;
 		FAST_PATH_SET_LOCKMODE(MyProc, unused_slot, lockmode);
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 49204f91a20..749a79d48ef 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -116,7 +116,7 @@ ProcGlobalShmemSize(void)
 	 * nicely aligned in each backend.
 	 */
 	fpLockBitsSize = MAXALIGN(FastPathLockGroupsPerBackend * sizeof(uint64));
-	fpRelIdSize = MAXALIGN(FastPathLockGroupsPerBackend * sizeof(Oid) * FP_LOCK_SLOTS_PER_GROUP);
+	fpRelIdSize = MAXALIGN(FastPathLockSlotsPerBackend() * sizeof(Oid));
 
 	size = add_size(size, mul_size(TotalProcs, (fpLockBitsSize + fpRelIdSize)));
 
@@ -231,7 +231,7 @@ InitProcGlobal(void)
 	 * shared memory and then divide that between backends.
 	 */
 	fpLockBitsSize = MAXALIGN(FastPathLockGroupsPerBackend * sizeof(uint64));
-	fpRelIdSize = MAXALIGN(FastPathLockGroupsPerBackend * sizeof(Oid) * FP_LOCK_SLOTS_PER_GROUP);
+	fpRelIdSize = MAXALIGN(FastPathLockSlotsPerBackend() * sizeof(Oid));
 
 	fpPtr = ShmemAlloc(TotalProcs * (fpLockBitsSize + fpRelIdSize));
 	MemSet(fpPtr, 0, TotalProcs * (fpLockBitsSize + fpRelIdSize));
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 318600d6d02..763893eed8f 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -586,7 +586,7 @@ InitializeFastPathLocks(void)
 	while (FastPathLockGroupsPerBackend < FP_LOCK_GROUPS_PER_BACKEND_MAX)
 	{
 		/* stop once we exceed max_locks_per_xact */
-		if (FastPathLockGroupsPerBackend * FP_LOCK_SLOTS_PER_GROUP >= max_locks_per_xact)
+		if (FastPathLockSlotsPerBackend() >= max_locks_per_xact)
 			break;
 
 		FastPathLockGroupsPerBackend *= 2;

 /*

---

From: Tomas Vondra <[email-masked]>
Date: Tue, 4 Mar 2025 19:58:38 +0100

On 3/4/25 15:38, Tomas Vondra wrote:
> ...

Pushed, with the tweaks to use FastPathLockSlotsPerBackend() in a couple
more places.

I noticed sifaka started failing right after I pushed this:

https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=sifaka&br=master

But I have no idea why would this cosmetic change cause issues with LDAP
tests, so I'm assuming the failure is unrelated, and the timing is
accidental and not caused by the patch.

regards

-- 
Tomas Vondra

---

From: Andres Freund <[email-masked]>
Date: Tue, 4 Mar 2025 14:09:24 -0500

Hi,

> Pushed, with the tweaks to use FastPathLockSlotsPerBackend() in a couple

Thanks!

> accidental and not caused by the patch.

The buildfarm was updated between those two runs.

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sifaka&dt=2025-03-04%2015%3A01%3A42
has
                                          'PGBuild::Log' => 'REL_18',
whereas the failing run
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sifaka&dt=2025-03-04%2017%3A35%3A40
has
                                          'PGBuild::Log' => 'REL_19',

It's worth noting that
a) sifaka doesn't build with ldap support
b) the failure is in checkprep, not when running the tests
c) the buildfarm unfortunately doesn't archive install.log, so it's hard to
   know what actually went wrong

Greetings,

Andres Freund

---

From: Tom Lane <[email-masked]>
Date: Tue, 04 Mar 2025 16:30:34 -0500

>    know what actually went wrong

Yeah, I've been poking at that.  It's not at all clear why the
animal is trying to run src/test/modules/ldap_password_func
now when it didn't before.  I've been through the diffs between
BF client 18 and 19 multiple times and nothing jumps out at me.

What's clear though is that it *is* trying to do "make check"
in that directory, and the link step blows up with

ccache clang -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Werror=unguarded-availability-new -Wendif-labels -Wmissing-format-attribute -Wcast-function-type -Wformat-security -Wmissing-variable-declarations -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-unused-command-line-argument -Wno-compound-token-split-by-macro -Wno-cast-function-type-strict -g -O2  -fvisibility=hidden -bundle -o ldap_password_func.dylib ldap_password_func.o  -L../../../../src/port -L../../../../src/common  -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX15.2.sdk  -L/opt/local/libexec/llvm-17/lib -L/opt/local/lib  -Wl,-dead_strip_dylibs   -fvisibility=hidden -bundle_loader ../../../../src/backend/postgres
Undefined symbols for architecture arm64:
  "_ldap_password_hook", referenced from:
      __PG_init in ldap_password_func.o
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

That happens because

(a) ldap_password_hook is not defined unless USE_LDAP;

(b) macOS's linker is persnickety and reports the missing symbol
at shlib link time, not shlib load time.

Maybe we should rethink (a)?  In the meantime I'm trying to hack
the script so it skips that test module, and finding out that
my Perl is rustier than I thought.

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Tue, 4 Mar 2025 16:46:51 -0500

Hi,

On 2025-03-04 16:30:34 -0500, Tom Lane wrote:
> now when it didn't before.

It did do so before as well, afaict:
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=sifaka&dt=2025-03-04%2015%3A01%3A42&stg=module-ldap_password_func-check

It seems to me that the difference is that now checkprep is run, whereas
previously it wasn't.

Before:
/Library/Developer/CommandLineTools/usr/bin/make -C adt jsonpath_gram.h
make[3]: `jsonpath_gram.h' is up to date.
echo "# +++ tap check in src/test/modules/ldap_password_func +++" && rm -rf '/Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/modules/ldap_password_func'/tmp_check && /bin/sh ../../../../config/install-sh -c -d '/Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/modules/ldap_password_func'/tmp_check && cd . && TESTLOGDIR='/Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/modules/ldap_password_func/tmp_check/log' TESTDATADIR='/Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/modules/ldap_password_func/tmp_check' PATH="/Users/buildfarm/bf-data/HEAD/pgsql.build/tmp_install/Users/buildfarm/bf-data/HEAD/inst/bin:/Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/modules/ldap_password_func:$PATH" DYLD_LIBRARY_PATH="/Users/buildfarm/bf-data/HEAD/pgsql.build/tmp_install/Users/buildfarm/bf-data/HEAD/inst/lib:$DYLD_LIBRARY_PATH" INITDB_TEMPLATE='/Users/buildfarm/bf-data/HEAD/pgsql.build'/tmp_install/initdb-template  PGPORT='65678' top_builddir='/Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/modules/ldap_password_func/../../../..' PG_REGRESS='/Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/modules/ldap_password_func/../../../../src/test/regress/pg_regress' share_contrib_dir='/Users/buildfarm/bf-data/HEAD/pgsql.build/tmp_install/Users/buildfarm/bf-data/HEAD/inst/share/postgresql/contrib' /usr/bin/prove -I ../../../../src/test/perl/ -I . --timer t/*.pl
# +++ tap check in src/test/modules/ldap_password_func +++
[10:08:59] t/001_mutated_bindpasswd.pl .. skipped: LDAP not supported by this build
[10:08:59]

Now:
/Library/Developer/CommandLineTools/usr/bin/make -C adt jsonpath_gram.h
make[3]: `jsonpath_gram.h' is up to date.
rm -rf '/Users/buildfarm/bf-data/HEAD/pgsql.build'/tmp_install
/bin/sh ../../../../config/install-sh -c -d '/Users/buildfarm/bf-data/HEAD/pgsql.build'/tmp_install/log
/Library/Developer/CommandLineTools/usr/bin/make -C '../../../..' DESTDIR='/Users/buildfarm/bf-data/HEAD/pgsql.build'/tmp_install install >'/Users/buildfarm/bf-data/HEAD/pgsql.build'/tmp_install/log/install.log 2>&1
/Library/Developer/CommandLineTools/usr/bin/make -j1  checkprep >>'/Users/buildfarm/bf-data/HEAD/pgsql.build'/tmp_install/log/install.log 2>&1
make: *** [temp-install] Error 2
log files for step module-ldap_password_funcCheck:

Note during a normal build ldap_password_func shouldn't be entered:
# Test runs an LDAP server, so only run if ldap is in PG_TEST_EXTRA
ifeq ($(with_ldap),yes)
ifneq (,$(filter ldap,$(PG_TEST_EXTRA)))
SUBDIRS += ldap_password_func
else
ALWAYS_SUBDIRS += ldap_password_func
endif
else
ALWAYS_SUBDIRS += ldap_password_func
endif

Which leads me to suspect that the difference might be related to
NO_TEMP_INSTALL not being set while it previously was. Which then triggers the
module being built, whereas it previously wasn't.

Of course relying on NO_TEMP_INSTALL preventing this from being built isn't
exactly reliable...

Greetings,

Andres Freund

---

From: Tom Lane <[email-masked]>
Date: Tue, 04 Mar 2025 17:01:18 -0500

> It seems to me that the difference is that now checkprep is run, whereas

Maybe, but still I don't see any changes in the BF client that'd
explain it.  The animal's configuration hasn't changed either;
the only non-comment diff in its buildfarm.conf is

@@ -374,7 +376,7 @@
 
        base_port => 5678,
 
-       modules => [qw(TestUpgrade TestDecoding)],
+       modules => [qw(TestUpgrade)],
 
        # settings used by run_branches.pl
        global => {

which I changed to follow the lead of build-farm.conf.sample.
But surely that wouldn't affect this!?

			regards, tom lane

---

From: Andrew Dunstan <[email-masked]>
Date: Tue, 4 Mar 2025 17:09:43 -0500

> the only non-comment diff in its buildfarm.conf is

I think I found a logic bug. Testing.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

---

From: Tom Lane <[email-masked]>
Date: Tue, 04 Mar 2025 17:28:55 -0500

> I think I found a logic bug. Testing.

Not sure what you are looking at, but I was trying to fix it
by making the loop over test modules skip unbuilt modules,
borrowing the test you added in v19 to skip unbuilt contrib
modules.  It's a little more complicated for the other modules
because some of them have no .c files to be built, and I could
not get that to work.  I eventually concluded that there's
something wrong with the "scalar glob()" idiom you used.
A bit of googling suggested "grep -e, glob()" instead, and
that seems to work for me.  sifaka seems happy with the
attached patch.

			regards, tom lane

--- run_build.pl~	2025-03-04 16:34:04.082252563 -0500
+++ run_build.pl	2025-03-04 16:35:25.967357487 -0500
@@ -2483,6 +2483,11 @@ sub run_misc_tests
 		my $testname = basename($testdir);
 		next if $testname =~ /ssl/ && !$using_ssl;
 		next unless -d "$testdir/t";
+
+		# can't test it if we haven't built it
+		next unless grep -e, glob("$testdir/*.o $testdir/*.obj")
+			or not grep -e, glob("$testdir/*.c");
+
 		next if $using_msvc && $testname eq 'pg_bsd_indent';
 		next unless step_wanted("module-$testname");
 		print time_str(), "running misc test module-$testname ...\n"
@@ -2496,7 +2501,7 @@ sub run_misc_tests
 		my $testname = basename($testdir);
 
 		# can't test it if we haven't built it
-		next unless scalar glob("$testdir/*.o $testdir/*.obj");
+		next unless grep -e, glob("$testdir/*.o $testdir/*.obj");
 
 		# skip sepgsql unless it's marked for testing
 		next if $testname eq 'sepgsql' && $ENV{PG_TEST_EXTRA} !~ /\bsepgsql\b/;

---

From: Tom Lane <[email-masked]>
Date: Tue, 04 Mar 2025 17:49:57 -0500

>> I think I found a logic bug. Testing.

Oh!  I bet you are looking at this 18-to-19 diff:

@@ -416,7 +416,8 @@ sub check_install_is_complete
 	{
 		$tmp_loc = "$tmp_loc/$install_dir";
 		$bindir = "$tmp_loc/bin";
-		$libdir = "$tmp_loc/lib/postgresql";
+		$libdir = "$tmp_loc/lib";
+		$libdir .= '/postgresql' unless $libdir =~ /postgres|pgsql/;
 		return (-d $bindir && -d $libdir);
 	}
 	elsif (-e "$build_dir/src/Makefile.global")    # i.e. not msvc
@@ -427,7 +428,8 @@ sub check_install_is_complete
 		chomp $suffix;
 		$tmp_loc = "$tmp_loc/$install_dir";
 		$bindir = "$tmp_loc/bin";
-		$libdir = "$tmp_loc/lib/postgresql";
+		$libdir = "$tmp_loc/lib";
+		$libdir .= '/postgresql' unless $libdir =~ /postgres|pgsql/;
 	}
 
I'd dismissed that because sifaka isn't running in a directory
that has "postgres" or "pgsql" in its path, but just now I looked
at the logs of one of these steps, and guess where it's installing:

/usr/bin/make -C '../../../..' DESTDIR='/Users/buildfarm/bf-data/HEAD/pgsql.build'/tmp_install install >'/Users/buildfarm/bf-data/HEAD/pgsql.build'/tmp_install/log/install.log 2>&1

I bet the "pgsql.build" name is confusing it into doing extra
installs.  This'd explain the impression I had that the test steps
were running a bit slower than they ought to.  If you check
sifaka's just-posted green run against its history, that run took
13:48 versus recent times of 10:35 or thereabouts, so we're definitely
eating a good deal of time someplace...

			regards, tom lane

---

From: Andrew Dunstan <[email-masked]>
Date: Tue, 4 Mar 2025 17:51:59 -0500

On 2025-03-04 Tu 5:28 PM, Tom Lane wrote:
> Andrew Dunstan<[email-masked]> writes:

I'm looking at something else, namely the attached.

Will check your patch out too.

--
Andrew Dunstan
EDB:https://www.enterprisedb.com
diff --git a/PGBuild/Utils.pm b/PGBuild/Utils.pm
index b97de92..d96dcec 100644
--- a/PGBuild/Utils.pm
+++ b/PGBuild/Utils.pm
@@ -417,7 +417,7 @@ sub check_install_is_complete
 		$tmp_loc = "$tmp_loc/$install_dir";
 		$bindir = "$tmp_loc/bin";
 		$libdir = "$tmp_loc/lib";
-		$libdir .= '/postgresql' unless $libdir =~ /postgres|pgsql/;
+		$libdir .= '/postgresql' unless $install_dir =~ /postgres|pgsql/;
 		return (-d $bindir && -d $libdir);
 	}
 	elsif (-e "$build_dir/src/Makefile.global")    # i.e. not msvc
@@ -429,7 +429,7 @@ sub check_install_is_complete
 		$tmp_loc = "$tmp_loc/$install_dir";
 		$bindir = "$tmp_loc/bin";
 		$libdir = "$tmp_loc/lib";
-		$libdir .= '/postgresql' unless $libdir =~ /postgres|pgsql/;
+		$libdir .= '/postgresql' unless $install_dir =~ /postgres|pgsql/;
 	}
 
 	# these files should be present if we've temp_installed everything,

---

From: Tom Lane <[email-masked]>
Date: Tue, 04 Mar 2025 18:04:34 -0500

> Will check your patch out too.

Comparing previous run against current, I now see that my patch
caused it to skip these steps:

module-ldap_password_func-check 
module-pg_bsd_indent-check 
contrib-sepgsql-check 

Skipping the ldap and sepgsql tests is desirable, but it shouldn't
have skipped pg_bsd_indent.  I think the cause of that is that
src/tools/pg_bsd_indent isn't built in any of the previous build
steps.  Up to now it got built as a side-effect of invoking the
tests, which isn't great because any build errors/warnings disappear
into the install log which the script doesn't capture.  I agree
with not capturing the install log, because that's generally
uninteresting once we get past make-install; but we have to be sure
that everything gets built before that.

			regards, tom lane

---

From: Andrew Dunstan <[email-masked]>
Date: Tue, 4 Mar 2025 18:25:53 -0500

> that seems to work for me.  sifaka seems happy with the

Well, in scalar context it should give us back the first item found, or 
undef if nothing is found, AIUI.

But you're right, it might read better if I use a different formulation.

I didn't much like this, though:

+

Too many negatives makes my head hurt.

I also note you said in a later email there were issues.

cheers

andrew

---

From: Tom Lane <[email-masked]>
Date: Tue, 04 Mar 2025 18:27:03 -0500

> I'm looking at something else, namely the attached.

Yeah, that avoids the extra installs and brings sifaka's
runtime back to about what it had been.

			regards, tom lane

---

From: Andrew Dunstan <[email-masked]>
Date: Tue, 4 Mar 2025 19:13:19 -0500

> that everything gets built before that.

Yeah ... I think an easy fix is to put this in make_testmodules():

+
+       # build pg_bsd_indent at the same time
+       # this doesn't really belong here, but it's convenient
+       if (-d "$pgsql/src/tools/pg_bsd_indent" && !$status)
+       {
+               my @indentout = run_log("cd 
$pgsql/src/tools/pg_bsd_indent && $make_cmd");
+               $status = $? >> 8;
+               push(@makeout,@indentout);
+       }

A lot of this special processing goes away when we're building with meson.

cheers

andrew

---

From: Tom Lane <[email-masked]>
Date: Tue, 04 Mar 2025 19:16:33 -0500

Andrew Dunstan <[email-masked]> writes:
>> ... I eventually concluded that there's

> undef if nothing is found, AIUI.

That's what I would have thought too, but it didn't seem to work that
way when I was testing the logic standalone: the script processed or
skipped directories according to no rule that I could figure out.

Anyway, for the moment I think we're all right with just the
directory path fix.

			regards, tom lane