Re: Optimizing FastPathTransferRelationLocks()

On Tue, Mar 11, 2025 at 8:46 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>
>
>
> 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(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.
>
> I’ve updated the patch as suggested. Updated patch is attached.
>

Thanks.

>
> > 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.

Fair enough. The code is more readable this way. That itself is an improvement.

I stared at c4d5cb71d229095a39fda1121a75ee40e6069a2a to see whether
there's any reason this was left aside at that time. I am convinced
that it was just missed. I think this patch is good to be committed.

--
Best Wishes,
Ashutosh Bapat