On 2024/10/21 16:32, Bertrand Drouvot wrote:
A few random comments on it:
Thanks for the review!
1 ===
+ /* Skip backends with pid=0, as they don't hold fast-path locks
*/
+ if (proc->pid == 0)
+ continue;
What about adding a few words in the comment that it represents prepared
transactions? Or maybe add a new macro (say IS_PREPARED_TRANSACTION(proc)) and
use it in the few places where we check for "PGPROC"->pid == 0 or "PGPROC"->pid
!= 0?
I understand that PGPROC entries with pid=0 are typically those not yet
allocated to
any backends. Yes, as you mentioned, prepared transactions also have pid=0.
However,
GetLockStatusData() loops up to ProcGlobal->allProcCount, which is MaxBackends
plus
NUM_AUXILIARY_PROCS, excluding prepared transactions. Therefore,
GetLockStatusData()
doesn't seem to check PGPROC entries for prepared transactions at all.
In proc.c
--------------
/* XXX allProcCount isn't really all of them; it excludes prepared xacts */
ProcGlobal->allProcCount = MaxBackends + NUM_AUXILIARY_PROCS;
--------------
One remark about the comment, what about?
s/Skip unallocated groups/Skip groups without registered fast-path locks./?
I've updated the source comment accordingly.
or at least add a "." at the end to be consistent with:
"/* Skip unallocated slots. */"
I removed the period at the end to match the usual convention in the codebase
for single-line comment.
I've attached v2 patch.
3 ===
One thing that worry me a bit is that we "lost" the FP_LOCK_SLOTS_PER_BACKEND
usage,
so that if there is a change on it (for wathever reason) then we probably need
to
be careful that the change would be reflected here too.
So, what about to add an Assert to check that we overall iterated over
FP_LOCK_SLOTS_PER_BACKEND?
You mean adding an assertion check to ensure that the slot ID calculated by
FAST_PATH_SLOT() is less than FP_LOCK_SLOTS_PER_BACKEND? But GetLockStatusData()
already calls FAST_PATH_GET_BITS() right after FAST_PATH_SLOT(),
and FAST_PATH_GET_BITS() has an assertion that validates this. So, probably
we can consider that this check is already in place. If it’s still worth adding,
perhaps placing it inside the FAST_PATH_SLOT() macro could be an option...
Or current assertion check is enough? Thought?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
From b55620cf73d12ef95dc21f3b42afaa264500612d Mon Sep 17 00:00:00 2001
From: Fujii Masao <fu...@postgresql.org>
Date: Sun, 20 Oct 2024 14:51:07 +0900
Subject: [PATCH v2] Refactor GetLockStatusData() to skip backends/groups
without fast-path locks.
Previously, GetLockStatusData() checked all slots for every backend
to gather fast-path lock data, which could be inefficient. This commit
refactors it by skipping backends with PID=0 (since they don't hold
fast-path locks) and skipping groups with no registered fast-path locks,
improving efficiency.
This refactoring is particularly beneficial, for example when
max_connections and max_locks_per_transaction are set high,
as it reduces unnecessary checks across numerous slots.
---
src/backend/storage/lmgr/lock.c | 67 +++++++++++++++++++--------------
1 file changed, 39 insertions(+), 28 deletions(-)
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index 09a8ac1578..4fccb7277e 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -3731,44 +3731,55 @@ GetLockStatusData(void)
for (i = 0; i < ProcGlobal->allProcCount; ++i)
{
PGPROC *proc = &ProcGlobal->allProcs[i];
- uint32 f;
+
+ /* Skip backends with pid=0, as they don't hold fast-path locks
*/
+ if (proc->pid == 0)
+ continue;
LWLockAcquire(&proc->fpInfoLock, LW_SHARED);
- for (f = 0; f < FP_LOCK_SLOTS_PER_BACKEND; ++f)
+ for (uint32 g = 0; g < FastPathLockGroupsPerBackend; g++)
{
- LockInstanceData *instance;
- uint32 lockbits = FAST_PATH_GET_BITS(proc, f);
-
- /* Skip unallocated slots. */
- if (!lockbits)
+ /* Skip groups without registered fast-path locks */
+ if (proc->fpLockBits[g] == 0)
continue;
- if (el >= els)
+ for (int j = 0; j < FP_LOCK_SLOTS_PER_GROUP; j++)
{
- els += MaxBackends;
- data->locks = (LockInstanceData *)
- repalloc(data->locks,
sizeof(LockInstanceData) * els);
- }
+ LockInstanceData *instance;
+ uint32 f = FAST_PATH_SLOT(g, j);
+ uint32 lockbits =
FAST_PATH_GET_BITS(proc, f);
- instance = &data->locks[el];
- SET_LOCKTAG_RELATION(instance->locktag,
proc->databaseId,
-
proc->fpRelId[f]);
- instance->holdMask = lockbits <<
FAST_PATH_LOCKNUMBER_OFFSET;
- instance->waitLockMode = NoLock;
- instance->vxid.procNumber = proc->vxid.procNumber;
- instance->vxid.localTransactionId = proc->vxid.lxid;
- instance->pid = proc->pid;
- instance->leaderPid = proc->pid;
- instance->fastpath = true;
+ /* Skip unallocated slots */
+ if (!lockbits)
+ continue;
- /*
- * Successfully taking fast path lock means there were
no
- * conflicting locks.
- */
- instance->waitStart = 0;
+ if (el >= els)
+ {
+ els += MaxBackends;
+ data->locks = (LockInstanceData *)
+ repalloc(data->locks,
sizeof(LockInstanceData) * els);
+ }
- el++;
+ instance = &data->locks[el];
+ SET_LOCKTAG_RELATION(instance->locktag,
proc->databaseId,
+
proc->fpRelId[f]);
+ instance->holdMask = lockbits <<
FAST_PATH_LOCKNUMBER_OFFSET;
+ instance->waitLockMode = NoLock;
+ instance->vxid.procNumber =
proc->vxid.procNumber;
+ instance->vxid.localTransactionId =
proc->vxid.lxid;
+ instance->pid = proc->pid;
+ instance->leaderPid = proc->pid;
+ instance->fastpath = true;
+
+ /*
+ * Successfully taking fast path lock means
there were no
+ * conflicting locks.
+ */
+ instance->waitStart = 0;
+
+ el++;
+ }
}
if (proc->fpVXIDLock)
--
2.46.2
