Re: Optimizing FastPathTransferRelationLocks()

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.

On Tue, Nov 19, 2024 at 10:14 PM Fujii Masao
<masao(dot)fujii(at)oss(dot)nttdata(dot)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.

@@ -2802,16 +2805,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)

I like this. Moving the group computation out of the loop also allows
the computed group to be used for checking existence of lock. That's
double benefit. This test is similar to the test in
GetLockStatusData(), so we already have a precedence.

>
>
> > And perhaps we should start clearing databaseid on backend exit.
>
> Maybe, but I'm not sure if we really need this..

There's a comment
* proc->databaseId is set at backend startup time and never changes
* thereafter, so it might be safe to perform this test before
* acquiring &proc->fpInfoLock.

that seems to assume that databaseid is never cleared, so maybe it's
not safe to do this.

The performance gain from this patch might be tiny and may not be
visible. Still, have you tried to measure the performance improvement?

--
Best Wishes,
Ashutosh Bapat