public inbox for [email protected]
help / color / mirror / Atom feedFrom: Fujii Masao <[email protected]>
To: Bertrand Drouvot <[email protected]>
Cc: [email protected]
Subject: Re: Refactor GetLockStatusData() by skipping unused backends and groups
Date: Wed, 23 Oct 2024 01:19:37 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
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 <[email protected]>
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
Attachments:
[text/plain] v2-0001-Refactor-GetLockStatusData-to-skip-backends-group.patch (3.5K, ../[email protected]/2-v2-0001-Refactor-GetLockStatusData-to-skip-backends-group.patch)
download | inline diff:
From b55620cf73d12ef95dc21f3b42afaa264500612d Mon Sep 17 00:00:00 2001
From: Fujii Masao <[email protected]>
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
view thread (5+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: Refactor GetLockStatusData() by skipping unused backends and groups
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox