On Thu, Dec 24, 2015 at 5:50 PM, Ildus Kurbangaliev < i.kurbangal...@postgrespro.ru> wrote: > > On Tue, 15 Dec 2015 13:56:30 -0500 > Robert Haas <robertmh...@gmail.com> wrote: > > > On Sun, Dec 13, 2015 at 6:35 AM, and...@anarazel.de > > <and...@anarazel.de> wrote: > > > On 2015-12-12 21:15:52 -0500, Robert Haas wrote: > > >> On Sat, Dec 12, 2015 at 1:17 PM, and...@anarazel.de > > >> <and...@anarazel.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