On Sun, Jun 16, 2024 at 09:28:05AM +0900, Michael Paquier wrote:
> On Thu, Jun 13, 2024 at 07:42:25PM -0700, Noah Misch wrote:
> > On Fri, Jun 14, 2024 at 09:58:59AM +0900, Michael Paquier wrote:
> >> GetWaitEventCustomIdentifier() is incorrect, and should return
> >> "InjectionPoint" in the default case of this class name, no?
> >
> > I intentionally didn't provide a default event ID for InjectionPoint.
> > PG_WAIT_EXTENSION needs a default case for backward compatibility, if
> > nothing
> > else. For this second custom type, it's needless complexity. The value
> > 0x0B000000U won't just show up like PG_WAIT_EXTENSION does.
> > GetLWLockIdentifier() also has no default case. How do you see it?
>
> I would add a default for consistency as this is just a few extra
> lines, but if you feel strongly about that, I'm OK as well. It makes
> a bit easier the detection of incorrect wait event numbers set
> incorrectly in extensions depending on the class wanted.
It would be odd to detect exactly 0x0B000000U and not other invalid inputs,
like 0x0A000001U where only 0x0B000001U is valid. I'm attaching roughly what
it would take. Shall I squash this into inplace031?
The thing I feel strongly about here is keeping focus on fixing $SUBJECT bugs
that are actually corrupting data out there. I think we should all limit our
interest in the verbiage of strings that appear only when running developer
tests, especially when $SUBJECT is a bug fix. When the string appears only
after C code passes invalid input to other C code, it matters even less.
> > The patch added to xfunc.sgml an example of using it. I'd be more inclined
> > to
> > delete the WaitEventExtensionNew() docbook documentation than to add its
> > level
> > of detail for WaitEventInjectionPointNew(). We don't have that kind of
> > documentation for most extension-facing C functions.
>
> It's one of the areas where I think that we should have more
> documentation, not less of it, so I'd rather keep it and maintaining
> it is not really a pain (?). The backend gets complicated enough
> these days that limiting what developers have to guess on their own is
> a better long-term approach because the Postgres out-of-core ecosystem
> is expanding a lot (aka have also in-core documentation for hooks,
> even if there's been a lot of reluctance historically about having
> them).
[getting deeply off topic -- let's move this to another thread if it needs to
expand] I like reducing the need to guess. So far in this inplace update
project (this thread plus postgr.es/m/20240615223718.42.nmi...@google.com),
three patches just fix comments. Even comments carry quite a price, but I
value them. When we hand-maintain documentation of a C function in both its
header comment and another place, I get skeptical about whether hackers
(including myself) will actually keep them in sync and skeptical of the
incremental value of maintaining the second version.
diff --git a/src/backend/utils/activity/wait_event.c
b/src/backend/utils/activity/wait_event.c
index 300de90..aaf9f3d 100644
--- a/src/backend/utils/activity/wait_event.c
+++ b/src/backend/utils/activity/wait_event.c
@@ -47,11 +47,11 @@ uint32 *my_wait_event_info =
&local_my_wait_event_info;
* Hash tables for storing custom wait event ids and their names in
* shared memory.
*
- * WaitEventCustomHashById is used to find the name from an event id.
- * Any backend can search it to find custom wait events.
+ * WaitEventCustomHashByInfo is used to find the name from wait event
+ * information. Any backend can search it to find custom wait events.
*
- * WaitEventCustomHashByName is used to find the ID from a name.
- * It is used to ensure that no duplicated entries are registered.
+ * WaitEventCustomHashByName is used to find the wait event information from a
+ * name. It is used to ensure that no duplicated entries are registered.
*
* For simplicity, we use the same ID counter across types of custom events.
* We could end that anytime the need arises.
@@ -61,18 +61,18 @@ uint32 *my_wait_event_info =
&local_my_wait_event_info;
* unlikely that the number of entries will reach
* WAIT_EVENT_CUSTOM_HASH_MAX_SIZE.
*/
-static HTAB *WaitEventCustomHashById; /* find names from IDs */
-static HTAB *WaitEventCustomHashByName; /* find IDs from names */
+static HTAB *WaitEventCustomHashByInfo; /* find names from infos */
+static HTAB *WaitEventCustomHashByName; /* find infos from names */
#define WAIT_EVENT_CUSTOM_HASH_INIT_SIZE 16
#define WAIT_EVENT_CUSTOM_HASH_MAX_SIZE 128
/* hash table entries */
-typedef struct WaitEventCustomEntryById
+typedef struct WaitEventCustomEntryByInfo
{
- uint16 event_id; /* hash key */
+ uint32 wait_event_info; /* hash key */
char wait_event_name[NAMEDATALEN]; /* custom wait event
name */
-} WaitEventCustomEntryById;
+} WaitEventCustomEntryByInfo;
typedef struct WaitEventCustomEntryByName
{
@@ -95,7 +95,7 @@ static WaitEventCustomCounterData *WaitEventCustomCounter;
#define WAIT_EVENT_CUSTOM_INITIAL_ID 1
static uint32 WaitEventCustomNew(uint32 classId, const char *wait_event_name);
-static const char *GetWaitEventCustomIdentifier(uint16 eventId);
+static const char *GetWaitEventCustomIdentifier(uint32 wait_event_info);
/*
* Return the space for dynamic shared hash tables and dynamic allocation
counter.
@@ -107,7 +107,7 @@ WaitEventCustomShmemSize(void)
sz = MAXALIGN(sizeof(WaitEventCustomCounterData));
sz = add_size(sz, hash_estimate_size(WAIT_EVENT_CUSTOM_HASH_MAX_SIZE,
-
sizeof(WaitEventCustomEntryById)));
+
sizeof(WaitEventCustomEntryByInfo)));
sz = add_size(sz, hash_estimate_size(WAIT_EVENT_CUSTOM_HASH_MAX_SIZE,
sizeof(WaitEventCustomEntryByName)));
return sz;
@@ -134,13 +134,13 @@ WaitEventCustomShmemInit(void)
}
/* initialize or attach the hash tables to store custom wait events */
- info.keysize = sizeof(uint16);
- info.entrysize = sizeof(WaitEventCustomEntryById);
- WaitEventCustomHashById = ShmemInitHash("WaitEventCustom hash by id",
-
WAIT_EVENT_CUSTOM_HASH_INIT_SIZE,
-
WAIT_EVENT_CUSTOM_HASH_MAX_SIZE,
-
&info,
-
HASH_ELEM | HASH_BLOBS);
+ info.keysize = sizeof(uint32);
+ info.entrysize = sizeof(WaitEventCustomEntryByInfo);
+ WaitEventCustomHashByInfo = ShmemInitHash("WaitEventCustom hash by wait
event information",
+
WAIT_EVENT_CUSTOM_HASH_INIT_SIZE,
+
WAIT_EVENT_CUSTOM_HASH_MAX_SIZE,
+
&info,
+
HASH_ELEM | HASH_BLOBS);
/* key is a NULL-terminated string */
info.keysize = sizeof(char[NAMEDATALEN]);
@@ -176,7 +176,7 @@ WaitEventCustomNew(uint32 classId, const char
*wait_event_name)
uint16 eventId;
bool found;
WaitEventCustomEntryByName *entry_by_name;
- WaitEventCustomEntryById *entry_by_id;
+ WaitEventCustomEntryByInfo *entry_by_info;
uint32 wait_event_info;
/* Check the limit of the length of the event name */
@@ -249,18 +249,18 @@ WaitEventCustomNew(uint32 classId, const char
*wait_event_name)
SpinLockRelease(&WaitEventCustomCounter->mutex);
/* Register the new wait event */
- entry_by_id = (WaitEventCustomEntryById *)
- hash_search(WaitEventCustomHashById, &eventId,
+ wait_event_info = classId | eventId;
+ entry_by_info = (WaitEventCustomEntryByInfo *)
+ hash_search(WaitEventCustomHashByInfo, &wait_event_info,
HASH_ENTER, &found);
Assert(!found);
- strlcpy(entry_by_id->wait_event_name, wait_event_name,
- sizeof(entry_by_id->wait_event_name));
+ strlcpy(entry_by_info->wait_event_name, wait_event_name,
+ sizeof(entry_by_info->wait_event_name));
entry_by_name = (WaitEventCustomEntryByName *)
hash_search(WaitEventCustomHashByName, wait_event_name,
HASH_ENTER, &found);
Assert(!found);
- wait_event_info = classId | eventId;
entry_by_name->wait_event_info = wait_event_info;
LWLockRelease(WaitEventCustomLock);
@@ -269,28 +269,29 @@ WaitEventCustomNew(uint32 classId, const char
*wait_event_name)
}
/*
- * Return the name of a custom wait event ID.
+ * Return the name of a custom wait event information.
*/
static const char *
-GetWaitEventCustomIdentifier(uint16 eventId)
+GetWaitEventCustomIdentifier(uint32 wait_event_info)
{
bool found;
- WaitEventCustomEntryById *entry;
+ WaitEventCustomEntryByInfo *entry;
/* Built-in event? */
- if (eventId < WAIT_EVENT_CUSTOM_INITIAL_ID)
+ if (wait_event_info == PG_WAIT_EXTENSION)
return "Extension";
/* It is a user-defined wait event, so lookup hash table. */
LWLockAcquire(WaitEventCustomLock, LW_SHARED);
- entry = (WaitEventCustomEntryById *)
- hash_search(WaitEventCustomHashById, &eventId,
+ entry = (WaitEventCustomEntryByInfo *)
+ hash_search(WaitEventCustomHashByInfo, &wait_event_info,
HASH_FIND, &found);
LWLockRelease(WaitEventCustomLock);
if (!entry)
- elog(ERROR, "could not find custom wait event name for ID %u",
- eventId);
+ elog(ERROR,
+ "could not find custom name for wait event information
%u",
+ wait_event_info);
return entry->wait_event_name;
}
@@ -449,7 +450,7 @@ pgstat_get_wait_event(uint32 wait_event_info)
break;
case PG_WAIT_EXTENSION:
case PG_WAIT_INJECTIONPOINT:
- event_name = GetWaitEventCustomIdentifier(eventId);
+ event_name =
GetWaitEventCustomIdentifier(wait_event_info);
break;
case PG_WAIT_BUFFERPIN:
{
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 5696604..75433b3 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -3100,7 +3100,7 @@ WaitEventActivity
WaitEventBufferPin
WaitEventClient
WaitEventCustomCounterData
-WaitEventCustomEntryById
+WaitEventCustomEntryByInfo
WaitEventCustomEntryByName
WaitEventIO
WaitEventIPC
