On 2025/03/11 20:55, Ashutosh Bapat wrote:
Hi Fujii-san,
It seems that this was forgotten somehow.
The patch still applies.
Examining c4d5cb71d229095a39fda1121a75ee40e6069a2a, it seems that this patch
could have been part of that commit as well. But may be it wasn't so apparent
that time. I think it's a good improvement.
Thanks for reviewing the patch!
On Tue, Nov 19, 2024 at 10:14 PM Fujii Masao
<masao.fu...@oss.nttdata.com> wrote:
The latest version of the patch is attached.
@@ -2773,6 +2773,10 @@ FastPathTransferRelationLocks(LockMethod
lockMethodTable, const LOCKTAG *locktag
LWLock *partitionLock = LockHashPartitionLock(hashcode);
Oid relid = locktag->locktag_field2;
uint32 i;
+ uint32 group;
+
+ /* fast-path group the lock belongs to */
+ group = FAST_PATH_REL_GROUP(relid);
We could just combine variable declaration and initialization; similar
to partitionLock.
I’ve updated the patch as suggested. Updated patch is attached.
The performance gain from this patch might be tiny and may not be
visible. Still, have you tried to measure the performance improvement?
I haven’t measured the actual performance gain since the patch optimizes
the logic in a clear and logical way. While we might see some improvement
in artificial scenarios — like with a large max_connections and
all backends slots having their databaseIds set, I’m not sure
how meaningful that would be.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
From e2d0cca4f9032aa5f5fbab024d23562633e2707f Mon Sep 17 00:00:00 2001
From: Fujii Masao <fu...@postgresql.org>
Date: Tue, 11 Mar 2025 22:31:12 +0900
Subject: [PATCH v3] Optimize iteration over PGPROC for fast-path lock
searches.
This commit improves efficiency in FastPathTransferRelationLocks()
and GetLockConflicts(), which iterate over PGPROCs to search for
fast-path locks.
Previously, these functions recalculated the fast-path group during
every loop iteration, even though it remained constant. This update
optimizes the process by calculating the group once and reusing it
throughout the loop.
The functions also now skip empty fast-path groups, avoiding
unnecessary scans of their slots. Additionally, groups belonging to
inactive backends (with pid=0) are always empty, so checking
the group is sufficient to bypass these backends, further enhancing
performance.
Author: Fujii Masao <masao.fu...@gmail.com>
Reviewed-by: Heikki Linnakangas <hlinn...@iki.fi>
Reviewed-by: Ashutosh Bapat <ashutosh.bapat....@gmail.com>
Discussion:
https://postgr.es/m/07d5fd6a-71f1-4ce8-8602-4cc6883f4...@oss.nttdata.com
---
src/backend/storage/lmgr/lock.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index ccfe6b69bf5..cb95fd74fcf 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -2774,6 +2774,9 @@ FastPathTransferRelationLocks(LockMethod lockMethodTable,
const LOCKTAG *locktag
Oid relid = locktag->locktag_field2;
uint32 i;
+ /* fast-path group the lock belongs to */
+ uint32 group = FAST_PATH_REL_GROUP(relid);
+
/*
* Every PGPROC that can potentially hold a fast-path lock is present in
* ProcGlobal->allProcs. Prepared transactions are not, but any
@@ -2783,8 +2786,7 @@ FastPathTransferRelationLocks(LockMethod lockMethodTable,
const LOCKTAG *locktag
for (i = 0; i < ProcGlobal->allProcCount; i++)
{
PGPROC *proc = &ProcGlobal->allProcs[i];
- uint32 j,
- group;
+ uint32 j;
LWLockAcquire(&proc->fpInfoLock, LW_EXCLUSIVE);
@@ -2802,16 +2804,16 @@ FastPathTransferRelationLocks(LockMethod
lockMethodTable, const LOCKTAG *locktag
* less clear that our backend is certain to have performed a
memory
* fencing operation since the other backend set
proc->databaseId. So
* for now, we test it after acquiring the LWLock just to be
safe.
+ *
+ * Also skip groups without any registered fast-path locks.
*/
- if (proc->databaseId != locktag->locktag_field1)
+ if (proc->databaseId != locktag->locktag_field1 ||
+ proc->fpLockBits[group] == 0)
{
LWLockRelease(&proc->fpInfoLock);
continue;
}
- /* fast-path group the lock belongs to */
- group = FAST_PATH_REL_GROUP(relid);
-
for (j = 0; j < FP_LOCK_SLOTS_PER_GROUP; j++)
{
uint32 lockmode;
@@ -3027,6 +3029,9 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE
lockmode, int *countp)
Oid relid = locktag->locktag_field2;
VirtualTransactionId vxid;
+ /* fast-path group the lock belongs to */
+ uint32 group = FAST_PATH_REL_GROUP(relid);
+
/*
* Iterate over relevant PGPROCs. Anything held by a prepared
* transaction will have been transferred to the primary lock
table,
@@ -3040,8 +3045,7 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE
lockmode, int *countp)
for (i = 0; i < ProcGlobal->allProcCount; i++)
{
PGPROC *proc = &ProcGlobal->allProcs[i];
- uint32 j,
- group;
+ uint32 j;
/* A backend never blocks itself */
if (proc == MyProc)
@@ -3056,16 +3060,16 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE
lockmode, int *countp)
*
* See FastPathTransferRelationLocks() for discussion
of why we do
* this test after acquiring the lock.
+ *
+ * Also skip groups without any registered fast-path
locks.
*/
- if (proc->databaseId != locktag->locktag_field1)
+ if (proc->databaseId != locktag->locktag_field1 ||
+ proc->fpLockBits[group] == 0)
{
LWLockRelease(&proc->fpInfoLock);
continue;
}
- /* fast-path group the lock belongs to */
- group = FAST_PATH_REL_GROUP(relid);
-
for (j = 0; j < FP_LOCK_SLOTS_PER_GROUP; j++)
{
uint32 lockmask;
--
2.48.1
