Re: [PATCH] Refactoring of LWLock tranches

On Thu, Feb 11, 2016 at 3:15 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> Sounds sensible, however after changes, CreateLWLocks() started
> looking unreadable, so moved initialization and registration of tranches
> to separate functions.

Seems good.

> Increased number of tranches allocated in LWLockTrancheArray, as
> now the LWTRANCHE_FIRST_USER_DEFINED is already greater
> than 16.

OK, cool.

+ int numIndividualLocks = NUM_INDIVIDUAL_LWLOCKS;
+ int numBufMappingLocks = NUM_BUFFER_PARTITIONS;
+ int numLmgrLocks = NUM_LOCK_PARTITIONS;
+ int i;

This doesn't seem like it's buying us anything. Can't we just use the
constants directly?

+ BufMappingLWLockTranche.name = "Buffer Mapping Locks";

We've not been very consistent about how we name our tranches. If
we're going to try to expose this information to users, we're going to
need to do better. We've got these names:

main
clog
commit_timestamp
subtrans
multixact_offset
multixact_member
async
oldserxid
WALInsertLocks
Buffer Content Locks
Buffer IO Locks
ReplicationOrigins
Replication Slot IO Locks
proc

Personally, I prefer the style of all lower case, words separated with
semicolons where necessary, the word "locks" left out. That way we
can display wait events as something like lwlock:<name of tranche> and
not have the word "lock" in there twice. I don't think it does much
good to try to make these names "user friendly" because,
fundamentally, these are internal pieces of the system and you won't
know what they are unless you do. So I'd prefer to make them look
like identifiers. If we adopt that style, then I think the new
tranches created by this patch should be named buffer_mapping,
lock_manager, and predicate_lock_manager and then, probably as a
separate patch, we should rename the rest to match that style
(buffer_content, buffer_io, replication_origin, replication_slot_io).

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company