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
