Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager

On Fri, Mar 13, 2020 at 2:32 PM Kuntal Ghosh <kuntalghosh(dot)2007(at)gmail(dot)com> wrote:
>
> On Fri, Mar 13, 2020 at 8:29 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Thu, Mar 12, 2020 at 7:50 PM Kuntal Ghosh <kuntalghosh(dot)2007(at)gmail(dot)com> wrote:
> > > I think moving them inside a macro is a good idea. Also, I think we
> > > should move all the Assert related code inside some debugging macro
> > > similar to this:
> > > #ifdef LOCK_DEBUG
> > > ....
> > > #endif
> > >
> > If we move it under some macro, then those Asserts will be only
> > enabled when that macro is defined. I think we want there Asserts to
> > be enabled always in assert enabled build, these will be like any
> > other Asserts in the code. What is the advantage of doing those under
> > macro?
> >
> My concern is related to performance regression. We're using two
> static variables in hot-paths only for checking a few asserts. So, I'm
> not sure whether we should enable the same by default, specially when
> asserts are itself disabled.
> -ResetRelExtLockHeldCount()
> +ResetRelExtPageLockHeldCount()
> {
> RelationExtensionLockHeldCount = 0;
> + PageLockHeldCount = 0;
> +}
> Also, we're calling this method from frequently used functions like
> Commit/AbortTransaction. So, it's better these two static variables
> share the same cache line and reinitalize them with a single
> instruction.

In the recent version of the patch, instead of a counter, we have done
with a flag. So I think now we can just keep a single variable and we
can just reset the bit in a single instruction.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com