On 3/3/25 21:52, Andres Freund wrote:
> Hi,
>
> On 2025-03-03 21:31:42 +0100, Tomas Vondra wrote:
>> On 3/3/25 19:10, Andres Freund wrote:
>>> On 2024-09-21 20:33:49 +0200, Tomas Vondra wrote:
>>>> I've finally pushed this, after many rounds of careful testing to ensure
>>>> 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.
>>>
>>
>> Fair point, although it can't change "run to run" - not without a
>> restart.
>
> That's what I meant with "run to run".
>
OK.
>
>> 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?
>
> 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.
>
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?
>
>> 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?
>
> That'd work for me.
>
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)
/*
* Flags for PGPROC.delayChkptFlags
