Re: [PATCH] Refactoring of LWLock tranches

On Thu, Dec 24, 2015 at 5:50 PM, Ildus Kurbangaliev <
i(dot)kurbangaliev(at)postgrespro(dot)ru> wrote:
>
> On Tue, 15 Dec 2015 13:56:30 -0500
> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> > On Sun, Dec 13, 2015 at 6:35 AM, andres(at)anarazel(dot)de
> > <andres(at)anarazel(dot)de> wrote:
> > > On 2015-12-12 21:15:52 -0500, Robert Haas wrote:
> > >> On Sat, Dec 12, 2015 at 1:17 PM, andres(at)anarazel(dot)de
> > >> <andres(at)anarazel(dot)de> wrote:
> > >> > Here's two patches doing that. The first is an adaption of your
> > >> > constants patch, using an enum and also converting xlog.c's
> > >> > locks. The second is the separation into distinct tranches.
> > >>
> > >> Personally, I prefer the #define approach to the enum, but I can
> > >> live with doing it this way.
> > >
> > > I think the lack needing to adjust the 'last defined' var is worth
> > > it...
> > >> Other than that, I think these patches look
> > >> good, although if it's OK with you I would like to make a pass over
> > >> the comments and the commit messages which seem to me that they
> > >> could benefit from a bit of editing (but not much substantive
> > >> change).
> > >
> > > Sounds good to me. You'll then commit that?
> >
> > Yes. Done!
> >
> > In terms of this project overall, NumLWLocks() now knows about only
> > four categories of stuff: fixed lwlocks, backend locks (proc.c),
> > replication slot locks, and locks needed by extensions. I think it'd
> > probably be fine to move the backend locks into PGPROC directly, and
> > the replication slot locks into ReplicationSlot. I don't know if that
> > will improve performance but it doesn't seem like it should regress
> > anything, though we should probably test that. I'm not sure what to
> > do about extension-requested locks - maybe give those their own
> > tranche somehow?
> >
> > I think we should also look at tranche-ifying the locks counted in
> > NUM_FIXED_LWLOCKS but not NUM_INDIVIDUAL_LWLOCKS. That's basically
> > just the lock manager locks and the predicate lock manager locks.
> > That would get us to a place where every lock in the system has a
> > descriptive name, either via the tranche or because it's an
> > individually named lock, which sounds excellent.
> >
>
> There is a patch that moves backend LWLocks into PGPROC and to a
> separate tranche.
>

1.
@@ -437,6 +440,13 @@ InitProcessPhase2(void)
{
Assert(MyProc != NULL);

+ /* Register and initialize fields of ProcLWLockTranche */
+ ProcLWLockTranche.name = "proc";
+ ProcLWLockTranche.array_base = (char *) (ProcGlobal->allProcs) +
+ offsetof(PGPROC, backendLock);
+ ProcLWLockTranche.array_stride = sizeof(PGPROC);
+ LWLockRegisterTranche(LWTRANCHE_PROC, &ProcLWLockTranche);
+

I think this will not work for Auxilary processes as they won't
call InitProcessPhase2(). It is better to initialize it in
InitProcGlobal() and then propagate it to backends for EXEC_BACKEND
cases as we do for ProcStructLock, AuxiliaryProcs.

2.
@@ -213,6 +213,7 @@ typedef enum BuiltinTrancheIds
LWTRANCHE_WAL_INSERT,
LWTRANCHE_BUFFER_CONTENT,
LWTRANCHE_BUFFER_IO_IN_PROGRESS,
+ LWTRANCHE_PROC,
LWTRANCHE_FIRST_USER_DEFINED
} BuiltinTrancheIds;

Other trancheids are based on the name of their corresponding
LWLock, don't you think it is better to name it as
LWTRANCHE_BACKEND for the sake of consistency? Also consider
changing name at other places in patch for this tranche.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com