Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jZ2gn-00080p-Nl for pgsql-hackers@arkaria.postgresql.org; Thu, 14 May 2020 01:29:34 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1jZ2gm-0002FN-Jx for pgsql-hackers@arkaria.postgresql.org; Thu, 14 May 2020 01:29:32 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jZ2gm-0002F4-A9 for pgsql-hackers@lists.postgresql.org; Thu, 14 May 2020 01:29:32 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jZ2gj-0008VF-FV for pgsql-hackers@lists.postgresql.org; Thu, 14 May 2020 01:29:31 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.14.4/8.14.4) with ESMTP id 04E1TPJd009057; Wed, 13 May 2020 21:29:25 -0400 From: Tom Lane To: pgsql-hackers@lists.postgresql.org cc: Isaac Morland , Alvaro Herrera , Robert Haas Subject: Re: Our naming of wait events is a disaster. In-reply-to: <28683.1589405363@sss.pgh.pa.us> References: <20200512202726.GA31125@alvherre.pgsql> <9871.1589321468@sss.pgh.pa.us> <5598.1589338488@sss.pgh.pa.us> <28683.1589405363@sss.pgh.pa.us> Comments: In-reply-to Tom Lane message dated "Wed, 13 May 2020 17:29:23 -0400" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <8967.1589419627.0@sss.pgh.pa.us> Date: Wed, 13 May 2020 21:29:25 -0400 Message-ID: <9056.1589419765@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <8967.1589419627.1@sss.pgh.pa.us> The attached patch doesn't actually change any LWLock names, but it is a useful start on that project. What it does is to get rid of the current scheme of dynamically registering the names of built-in LWLocks in favor of having a constant array of those names. It's completely foolish to expend process-startup cycles on constructing an array of constant data; moreover, the way things are done now results in the tranche names being defined all over creation. I draw a short straight line between that technique and the lack of consistency in the tranche names. Given that we have an enum in lwlock.h enumerating the built-in tranches, there's certainly no expectation that somebody's going to create a new one without letting the lwlock module know about it, so this gives up no flexibility. In fact, it adds some, because we can now name an SLRU's buffer-locks tranche whatever we want --- it's not hard-wired as being the same as the SLRU's base name. The dynamic registration mechanism is still there, but it's now *only* used if you load an extension that creates dynamic LWLocks. At some point it might be interesting to generate the enum BuiltinTrancheIds and the BuiltinTrancheNames array from a common source file, as we do for lwlocknames.h/.c. I didn't feel a need to make that happen today, though. regards, tom lane ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="simplify-management-of-builtin-lwlocks.patch"; charset="us-ascii" Content-ID: <8967.1589419627.2@sss.pgh.pa.us> Content-Description: simplify-management-of-builtin-lwlocks.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transa= m/slru.c index 3ba9fc9..3572b01 100644 --- a/src/backend/access/transam/slru.c +++ b/src/backend/access/transam/slru.c @@ -235,9 +235,6 @@ SimpleLruInit(SlruCtl ctl, const char *name, int nslot= s, int nlsns, else Assert(found); = - /* Register SLRU tranche in the main tranches array */ - LWLockRegisterTranche(tranche_id, name); - /* * Initialize the unshared control struct, including directory path. We * assume caller set PagePrecedes. diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transa= m/xlog.c index a53e6d9..4284659 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -5116,10 +5116,8 @@ XLOGShmemInit(void) /* both should be present or neither */ Assert(foundCFile && foundXLog); = - /* Initialize local copy of WALInsertLocks and register the tranche */ + /* Initialize local copy of WALInsertLocks */ WALInsertLocks =3D XLogCtl->Insert.WALInsertLocks; - LWLockRegisterTranche(LWTRANCHE_WAL_INSERT, - "wal_insert"); = if (localControlFile) pfree(localControlFile); @@ -5155,7 +5153,6 @@ XLOGShmemInit(void) (WALInsertLockPadded *) allocptr; allocptr +=3D sizeof(WALInsertLockPadded) * NUM_XLOGINSERT_LOCKS; = - LWLockRegisterTranche(LWTRANCHE_WAL_INSERT, "wal_insert"); for (i =3D 0; i < NUM_XLOGINSERT_LOCKS; i++) { LWLockInitialize(&WALInsertLocks[i].l.lock, LWTRANCHE_WAL_INSERT); diff --git a/src/backend/replication/logical/origin.c b/src/backend/replic= ation/logical/origin.c index d0d2b46..923ea3f 100644 --- a/src/backend/replication/logical/origin.c +++ b/src/backend/replication/logical/origin.c @@ -517,9 +517,6 @@ ReplicationOriginShmemInit(void) ConditionVariableInit(&replication_states[i].origin_cv); } } - - LWLockRegisterTranche(replication_states_ctl->tranche_id, - "replication_origin"); } = /* ----------------------------------------------------------------------= ----- diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot= .c index abae74c..d3d1033 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -140,9 +140,6 @@ ReplicationSlotsShmemInit(void) ShmemInitStruct("ReplicationSlot Ctl", ReplicationSlotsShmemSize(), &found); = - LWLockRegisterTranche(LWTRANCHE_REPLICATION_SLOT_IO_IN_PROGRESS, - "replication_slot_io"); - if (!found) { int i; diff --git a/src/backend/storage/buffer/buf_init.c b/src/backend/storage/b= uffer/buf_init.c index af62d48..8954856 100644 --- a/src/backend/storage/buffer/buf_init.c +++ b/src/backend/storage/buffer/buf_init.c @@ -87,9 +87,6 @@ InitBufferPool(void) NBuffers * (Size) sizeof(LWLockMinimallyPadded), &foundIOLocks); = - LWLockRegisterTranche(LWTRANCHE_BUFFER_IO_IN_PROGRESS, "buffer_io"); - LWLockRegisterTranche(LWTRANCHE_BUFFER_CONTENT, "buffer_content"); - /* * The array used to sort to-be-checkpointed buffer ids is located in * shared memory, to avoid having to allocate significant amounts of diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc= /procarray.c index 3630006..6a94448 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -267,8 +267,6 @@ CreateSharedProcArray(void) mul_size(sizeof(bool), TOTAL_MAX_CACHED_SUBXIDS), &found); } - - LWLockRegisterTranche(LWTRANCHE_PROC, "proc"); } = /* diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/= lwlock.c index 4c14e51..a4bc47a 100644 --- a/src/backend/storage/lmgr/lwlock.c +++ b/src/backend/storage/lmgr/lwlock.c @@ -108,14 +108,85 @@ extern slock_t *ShmemLock; #define LW_SHARED_MASK ((uint32) ((1 << 24)-1)) = /* - * This is indexed by tranche ID and stores the names of all tranches kno= wn - * to the current backend. + * There are three sorts of LWLock "tranches": + * + * 1. The individually-named locks defined in lwlocknames.h each have the= ir + * own tranche. The names of these tranches appear in MainLWLockNames[] + * in lwlocknames.c. + * + * 2. There are some predefined tranches for built-in groups of locks. + * These are listed in enum BuiltinTrancheIds in lwlock.h, and their name= s + * appear in BuiltinTrancheNames[] below. + * + * 3. Extensions can create new tranches, via either RequestNamedLWLockTr= anche + * or LWLockRegisterTranche. The names of these that are known in the cu= rrent + * process appear in LWLockTrancheNames[]. */ -static const char **LWLockTrancheArray =3D NULL; -static int LWLockTranchesAllocated =3D 0; = -#define T_NAME(lock) \ - (LWLockTrancheArray[(lock)->tranche]) +static const char *const BuiltinTrancheNames[] =3D { + /* LWTRANCHE_CLOG_BUFFERS: */ + "clog", + /* LWTRANCHE_COMMITTS_BUFFERS: */ + "commit_timestamp", + /* LWTRANCHE_SUBTRANS_BUFFERS: */ + "subtrans", + /* LWTRANCHE_MXACTOFFSET_BUFFERS: */ + "multixact_offset", + /* LWTRANCHE_MXACTMEMBER_BUFFERS: */ + "multixact_member", + /* LWTRANCHE_ASYNC_BUFFERS: */ + "async", + /* LWTRANCHE_OLDSERXID_BUFFERS: */ + "oldserxid", + /* LWTRANCHE_WAL_INSERT: */ + "wal_insert", + /* LWTRANCHE_BUFFER_CONTENT: */ + "buffer_content", + /* LWTRANCHE_BUFFER_IO_IN_PROGRESS: */ + "buffer_io", + /* LWTRANCHE_REPLICATION_ORIGIN: */ + "replication_origin", + /* LWTRANCHE_REPLICATION_SLOT_IO_IN_PROGRESS: */ + "replication_slot_io", + /* LWTRANCHE_PROC: */ + "proc", + /* LWTRANCHE_BUFFER_MAPPING: */ + "buffer_mapping", + /* LWTRANCHE_LOCK_MANAGER: */ + "lock_manager", + /* LWTRANCHE_PREDICATE_LOCK_MANAGER: */ + "predicate_lock_manager", + /* LWTRANCHE_PARALLEL_HASH_JOIN: */ + "parallel_hash_join", + /* LWTRANCHE_PARALLEL_QUERY_DSA: */ + "parallel_query_dsa", + /* LWTRANCHE_SESSION_DSA: */ + "session_dsa", + /* LWTRANCHE_SESSION_RECORD_TABLE: */ + "session_record_table", + /* LWTRANCHE_SESSION_TYPMOD_TABLE: */ + "session_typmod_table", + /* LWTRANCHE_SHARED_TUPLESTORE: */ + "shared_tuplestore", + /* LWTRANCHE_TBM: */ + "tbm", + /* LWTRANCHE_PARALLEL_APPEND: */ + "parallel_append", + /* LWTRANCHE_SXACT: */ + "serializable_xact" +}; + +StaticAssertDecl(lengthof(BuiltinTrancheNames) =3D=3D + LWTRANCHE_FIRST_USER_DEFINED - NUM_INDIVIDUAL_LWLOCKS, + "missing entries in BuiltinTrancheNames[]"); + +/* + * This is indexed by tranche ID minus LWTRANCHE_FIRST_USER_DEFINED, and + * stores the names of all dynamically-created tranches known to the curr= ent + * process. Any unused entries in the array will contain NULL. + */ +static const char **LWLockTrancheNames =3D NULL; +static int LWLockTrancheNamesAllocated =3D 0; = /* * This points to the main array of LWLocks in shared memory. Backends i= nherit @@ -158,10 +229,13 @@ NamedLWLockTranche *NamedLWLockTrancheArray =3D NULL= ; static bool lock_named_request_allowed =3D true; = static void InitializeLWLocks(void); -static void RegisterLWLockTranches(void); = static inline void LWLockReportWaitStart(LWLock *lock); static inline void LWLockReportWaitEnd(void); +static const char *GetLWTrancheName(uint16 trancheId); + +#define T_NAME(lock) \ + GetLWTrancheName((lock)->tranche) = #ifdef LWLOCK_STATS typedef struct lwlock_stats_key @@ -332,7 +406,7 @@ get_lwlock_stats_entry(LWLock *lock) * allocated in the main array. */ static int -NumLWLocksByNamedTranches(void) +NumLWLocksForNamedTranches(void) { int numLocks =3D 0; int i; @@ -353,7 +427,8 @@ LWLockShmemSize(void) int i; int numLocks =3D NUM_FIXED_LWLOCKS; = - numLocks +=3D NumLWLocksByNamedTranches(); + /* Calculate total number of locks needed in the main array. */ + numLocks +=3D NumLWLocksForNamedTranches(); = /* Space for the LWLock array. */ size =3D mul_size(numLocks, sizeof(LWLockPadded)); @@ -368,7 +443,7 @@ LWLockShmemSize(void) for (i =3D 0; i < NamedLWLockTrancheRequests; i++) size =3D add_size(size, strlen(NamedLWLockTrancheRequestArray[i].tranch= e_name) + 1); = - /* Disallow named LWLocks' requests after startup */ + /* Disallow adding any more named tranches. */ lock_named_request_allowed =3D false; = return size; @@ -376,7 +451,7 @@ LWLockShmemSize(void) = /* * Allocate shmem space for the main LWLock array and all tranches and - * initialize it. We also register all the LWLock tranches here. + * initialize it. We also register extension LWLock tranches here. */ void CreateLWLocks(void) @@ -416,8 +491,10 @@ CreateLWLocks(void) InitializeLWLocks(); } = - /* Register all LWLock tranches */ - RegisterLWLockTranches(); + /* Register named extension LWLock tranches in the current process. */ + for (int i =3D 0; i < NamedLWLockTrancheRequests; i++) + LWLockRegisterTranche(NamedLWLockTrancheArray[i].trancheId, + NamedLWLockTrancheArray[i].trancheName); } = /* @@ -426,7 +503,7 @@ CreateLWLocks(void) static void InitializeLWLocks(void) { - int numNamedLocks =3D NumLWLocksByNamedTranches(); + int numNamedLocks =3D NumLWLocksForNamedTranches(); int id; int i; int j; @@ -452,7 +529,10 @@ InitializeLWLocks(void) for (id =3D 0; id < NUM_PREDICATELOCK_PARTITIONS; id++, lock++) LWLockInitialize(&lock->lock, LWTRANCHE_PREDICATE_LOCK_MANAGER); = - /* Initialize named tranches. */ + /* + * Copy the info about any named tranches into shared memory (so that + * other processes can see it), and initialize the requested LWLocks. + */ if (NamedLWLockTrancheRequests > 0) { char *trancheNames; @@ -486,51 +566,6 @@ InitializeLWLocks(void) } = /* - * Register named tranches and tranches for fixed LWLocks. - */ -static void -RegisterLWLockTranches(void) -{ - int i; - - if (LWLockTrancheArray =3D=3D NULL) - { - LWLockTranchesAllocated =3D 128; - LWLockTrancheArray =3D (const char **) - MemoryContextAllocZero(TopMemoryContext, - LWLockTranchesAllocated * sizeof(char *)); - Assert(LWLockTranchesAllocated >=3D LWTRANCHE_FIRST_USER_DEFINED); - } - - for (i =3D 0; i < NUM_INDIVIDUAL_LWLOCKS; ++i) - LWLockRegisterTranche(i, MainLWLockNames[i]); - - LWLockRegisterTranche(LWTRANCHE_BUFFER_MAPPING, "buffer_mapping"); - LWLockRegisterTranche(LWTRANCHE_LOCK_MANAGER, "lock_manager"); - LWLockRegisterTranche(LWTRANCHE_PREDICATE_LOCK_MANAGER, - "predicate_lock_manager"); - LWLockRegisterTranche(LWTRANCHE_PARALLEL_QUERY_DSA, - "parallel_query_dsa"); - LWLockRegisterTranche(LWTRANCHE_SESSION_DSA, - "session_dsa"); - LWLockRegisterTranche(LWTRANCHE_SESSION_RECORD_TABLE, - "session_record_table"); - LWLockRegisterTranche(LWTRANCHE_SESSION_TYPMOD_TABLE, - "session_typmod_table"); - LWLockRegisterTranche(LWTRANCHE_SHARED_TUPLESTORE, - "shared_tuplestore"); - LWLockRegisterTranche(LWTRANCHE_TBM, "tbm"); - LWLockRegisterTranche(LWTRANCHE_PARALLEL_APPEND, "parallel_append"); - LWLockRegisterTranche(LWTRANCHE_PARALLEL_HASH_JOIN, "parallel_hash_join"= ); - LWLockRegisterTranche(LWTRANCHE_SXACT, "serializable_xact"); - - /* Register named tranches. */ - for (i =3D 0; i < NamedLWLockTrancheRequests; i++) - LWLockRegisterTranche(NamedLWLockTrancheArray[i].trancheId, - NamedLWLockTrancheArray[i].trancheName); -} - -/* * InitLWLockAccess - initialize backend-local state needed to hold LWLoc= ks */ void @@ -595,32 +630,47 @@ LWLockNewTrancheId(void) } = /* - * Register a tranche ID in the lookup table for the current process. Th= is - * routine will save a pointer to the tranche name passed as an argument, + * Register a dynamic tranche name in the lookup table of the current pro= cess. + * + * This routine will save a pointer to the tranche name passed as an argu= ment, * so the name should be allocated in a backend-lifetime context - * (TopMemoryContext, static variable, or similar). + * (TopMemoryContext, static constant, or similar). */ void LWLockRegisterTranche(int tranche_id, const char *tranche_name) { - Assert(LWLockTrancheArray !=3D NULL); + /* This should only be called for user-defined tranches. */ + if (tranche_id < LWTRANCHE_FIRST_USER_DEFINED) + return; + + /* Convert to array index. */ + tranche_id -=3D LWTRANCHE_FIRST_USER_DEFINED; = - if (tranche_id >=3D LWLockTranchesAllocated) + /* If necessary, create or enlarge array. */ + if (tranche_id >=3D LWLockTrancheNamesAllocated) { - int i =3D LWLockTranchesAllocated; - int j =3D LWLockTranchesAllocated; + int newalloc; = - while (i <=3D tranche_id) - i *=3D 2; + newalloc =3D Max(LWLockTrancheNamesAllocated, 8); + while (newalloc <=3D tranche_id) + newalloc *=3D 2; = - LWLockTrancheArray =3D (const char **) - repalloc(LWLockTrancheArray, i * sizeof(char *)); - LWLockTranchesAllocated =3D i; - while (j < LWLockTranchesAllocated) - LWLockTrancheArray[j++] =3D NULL; + if (LWLockTrancheNames =3D=3D NULL) + LWLockTrancheNames =3D (const char **) + MemoryContextAllocZero(TopMemoryContext, + newalloc * sizeof(char *)); + else + { + LWLockTrancheNames =3D (const char **) + repalloc(LWLockTrancheNames, newalloc * sizeof(char *)); + memset(LWLockTrancheNames + LWLockTrancheNamesAllocated, + 0, + (newalloc - LWLockTrancheNamesAllocated) * sizeof(char *)); + } + LWLockTrancheNamesAllocated =3D newalloc; } = - LWLockTrancheArray[tranche_id] =3D tranche_name; + LWLockTrancheNames[tranche_id] =3D tranche_name; } = /* @@ -667,7 +717,7 @@ RequestNamedLWLockTranche(const char *tranche_name, in= t num_lwlocks) = request =3D &NamedLWLockTrancheRequestArray[NamedLWLockTrancheRequests]; Assert(strlen(tranche_name) + 1 < NAMEDATALEN); - StrNCpy(request->tranche_name, tranche_name, NAMEDATALEN); + strlcpy(request->tranche_name, tranche_name, NAMEDATALEN); request->num_lwlocks =3D num_lwlocks; NamedLWLockTrancheRequests++; } @@ -709,23 +759,42 @@ LWLockReportWaitEnd(void) } = /* - * Return an identifier for an LWLock based on the wait class and event. + * Return the name of an LWLock tranche. */ -const char * -GetLWLockIdentifier(uint32 classId, uint16 eventId) +static const char * +GetLWTrancheName(uint16 trancheId) { - Assert(classId =3D=3D PG_WAIT_LWLOCK); + /* Individual LWLock? */ + if (trancheId < NUM_INDIVIDUAL_LWLOCKS) + return MainLWLockNames[trancheId]; + + /* Built-in tranche? */ + if (trancheId < LWTRANCHE_FIRST_USER_DEFINED) + return BuiltinTrancheNames[trancheId - NUM_INDIVIDUAL_LWLOCKS]; = /* - * It is quite possible that user has registered tranche in one of the - * backends (e.g. by allocating lwlocks in dynamic shared memory) but no= t - * all of them, so we can't assume the tranche is registered here. + * It's an extension tranche, so look in LWLockTrancheNames[]. However, + * it's possible that the tranche has never been registered in the curre= nt + * process, in which case give up and return "extension". */ - if (eventId >=3D LWLockTranchesAllocated || - LWLockTrancheArray[eventId] =3D=3D NULL) + trancheId -=3D LWTRANCHE_FIRST_USER_DEFINED; + + if (trancheId >=3D LWLockTrancheNamesAllocated || + LWLockTrancheNames[trancheId] =3D=3D NULL) return "extension"; = - return LWLockTrancheArray[eventId]; + return LWLockTrancheNames[trancheId]; +} + +/* + * Return an identifier for an LWLock based on the wait class and event. + */ +const char * +GetLWLockIdentifier(uint32 classId, uint16 eventId) +{ + Assert(classId =3D=3D PG_WAIT_LWLOCK); + /* The event IDs are just tranche numbers. */ + return GetLWTrancheName(eventId); } = /* ------- =_aaaaaaaaaa0--