From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
Cc: Isaac Morland <isaac.morland@gmail.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Subject: Re: Our naming of wait events is a disaster.
Date: Wed, 13 May 2020 21:29:25 -0400
Message-ID: <9056.1589419765@sss.pgh.pa.us> (raw)
In-Reply-To: <28683.1589405363@sss.pgh.pa.us>
References: <20200512202726.GA31125@alvherre.pgsql>
<9871.1589321468@sss.pgh.pa.us>
<CAMsGm5ci68RWekX+JCA9QJzoqBk0Q08rJa2akv0eR1mvpURA2w@mail.gmail.com>
<5598.1589338488@sss.pgh.pa.us>
<28683.1589405363@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
Attachments:
[text/x-diff] simplify-management-of-builtin-lwlocks.patch (14.9K, ../9056.1589419765@sss.pgh.pa.us/2-simplify-management-of-builtin-lwlocks.patch)
download | inline diff:diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.cindex 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 nslots, 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/transam/xlog.cindex 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 = XLogCtl->Insert.WALInsertLocks;
- LWLockRegisterTranche(LWTRANCHE_WAL_INSERT,- "wal_insert");
if (localControlFile)
pfree(localControlFile);
@@ -5155,7 +5153,6 @@ XLOGShmemInit(void)
(WALInsertLockPadded *) allocptr;
allocptr += sizeof(WALInsertLockPadded) * NUM_XLOGINSERT_LOCKS;
- LWLockRegisterTranche(LWTRANCHE_WAL_INSERT, "wal_insert");
for (i = 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/replication/logical/origin.cindex 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.cindex 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/buffer/buf_init.cindex 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.cindex 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.cindex 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 known- * to the current backend.+ * There are three sorts of LWLock "tranches":+ *+ * 1. The individually-named locks defined in lwlocknames.h each have their+ * 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 names+ * appear in BuiltinTrancheNames[] below.+ *+ * 3. Extensions can create new tranches, via either RequestNamedLWLockTranche+ * or LWLockRegisterTranche. The names of these that are known in the current+ * process appear in LWLockTrancheNames[].
*/
-static const char **LWLockTrancheArray = NULL;-static int LWLockTranchesAllocated = 0;-#define T_NAME(lock) \- (LWLockTrancheArray[(lock)->tranche])+static const char *const BuiltinTrancheNames[] = {+ /* 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) ==+ 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 current+ * process. Any unused entries in the array will contain NULL.+ */+static const char **LWLockTrancheNames = NULL;+static int LWLockTrancheNamesAllocated = 0;
/*
* This points to the main array of LWLocks in shared memory. Backends inherit
@@ -158,10 +229,13 @@ NamedLWLockTranche *NamedLWLockTrancheArray = NULL;
static bool lock_named_request_allowed = 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 = 0;
int i;
@@ -353,7 +427,8 @@ LWLockShmemSize(void)
int i;
int numLocks = NUM_FIXED_LWLOCKS;
- numLocks += NumLWLocksByNamedTranches();+ /* Calculate total number of locks needed in the main array. */+ numLocks += NumLWLocksForNamedTranches();
/* Space for the LWLock array. */
size = mul_size(numLocks, sizeof(LWLockPadded));
@@ -368,7 +443,7 @@ LWLockShmemSize(void)
for (i = 0; i < NamedLWLockTrancheRequests; i++)
size = add_size(size, strlen(NamedLWLockTrancheRequestArray[i].tranche_name) + 1);
- /* Disallow named LWLocks' requests after startup */+ /* Disallow adding any more named tranches. */
lock_named_request_allowed = 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 = 0; i < NamedLWLockTrancheRequests; i++)+ LWLockRegisterTranche(NamedLWLockTrancheArray[i].trancheId,+ NamedLWLockTrancheArray[i].trancheName);
}
/*
@@ -426,7 +503,7 @@ CreateLWLocks(void)
static void
InitializeLWLocks(void)
{
- int numNamedLocks = NumLWLocksByNamedTranches();+ int numNamedLocks = NumLWLocksForNamedTranches();
int id;
int i;
int j;
@@ -452,7 +529,10 @@ InitializeLWLocks(void)
for (id = 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 == NULL)- {- LWLockTranchesAllocated = 128;- LWLockTrancheArray = (const char **)- MemoryContextAllocZero(TopMemoryContext,- LWLockTranchesAllocated * sizeof(char *));- Assert(LWLockTranchesAllocated >= LWTRANCHE_FIRST_USER_DEFINED);- }-- for (i = 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 = 0; i < NamedLWLockTrancheRequests; i++)- LWLockRegisterTranche(NamedLWLockTrancheArray[i].trancheId,- NamedLWLockTrancheArray[i].trancheName);-}--/*
* InitLWLockAccess - initialize backend-local state needed to hold LWLocks
*/
void
@@ -595,32 +630,47 @@ LWLockNewTrancheId(void)
}
/*
- * Register a tranche ID in the lookup table for the current process. This- * 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 process.+ *+ * This routine will save a pointer to the tranche name passed as an argument,
* 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 != NULL);+ /* This should only be called for user-defined tranches. */+ if (tranche_id < LWTRANCHE_FIRST_USER_DEFINED)+ return;++ /* Convert to array index. */+ tranche_id -= LWTRANCHE_FIRST_USER_DEFINED;- if (tranche_id >= LWLockTranchesAllocated)+ /* If necessary, create or enlarge array. */+ if (tranche_id >= LWLockTrancheNamesAllocated)
{
- int i = LWLockTranchesAllocated;- int j = LWLockTranchesAllocated;+ int newalloc;- while (i <= tranche_id)- i *= 2;+ newalloc = Max(LWLockTrancheNamesAllocated, 8);+ while (newalloc <= tranche_id)+ newalloc *= 2;- LWLockTrancheArray = (const char **)- repalloc(LWLockTrancheArray, i * sizeof(char *));- LWLockTranchesAllocated = i;- while (j < LWLockTranchesAllocated)- LWLockTrancheArray[j++] = NULL;+ if (LWLockTrancheNames == NULL)+ LWLockTrancheNames = (const char **)+ MemoryContextAllocZero(TopMemoryContext,+ newalloc * sizeof(char *));+ else+ {+ LWLockTrancheNames = (const char **)+ repalloc(LWLockTrancheNames, newalloc * sizeof(char *));+ memset(LWLockTrancheNames + LWLockTrancheNamesAllocated,+ 0,+ (newalloc - LWLockTrancheNamesAllocated) * sizeof(char *));+ }+ LWLockTrancheNamesAllocated = newalloc;
}
- LWLockTrancheArray[tranche_id] = tranche_name;+ LWLockTrancheNames[tranche_id] = tranche_name;
}
/*
@@ -667,7 +717,7 @@ RequestNamedLWLockTranche(const char *tranche_name, int num_lwlocks)
request = &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 = 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 == 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 not- * 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 current+ * process, in which case give up and return "extension".
*/
- if (eventId >= LWLockTranchesAllocated ||- LWLockTrancheArray[eventId] == NULL)+ trancheId -= LWTRANCHE_FIRST_USER_DEFINED;++ if (trancheId >= LWLockTrancheNamesAllocated ||+ LWLockTrancheNames[trancheId] == 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 == PG_WAIT_LWLOCK);+ /* The event IDs are just tranche numbers. */+ return GetLWTrancheName(eventId);
}
/*
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org, isaac.morland@gmail.com, alvherre@2ndquadrant.com, robertmhaas@gmail.com
Subject: Re: Our naming of wait events is a disaster.
In-Reply-To: <9056.1589419765@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox