Re: Support to define custom wait events for extensions

On Thu, Jul 27, 2023 at 06:29:22PM +0900, Masahiro Ikeda wrote:
> I suspect that I forgot to specify "volatile" to the variable
> for the spinlock.

+ if (!IsUnderPostmaster)
+ {
+ /* Allocate space in shared memory. */
+ waitEventExtensionCounter = (WaitEventExtensionCounter *)
+ ShmemInitStruct("waitEventExtensionCounter", WaitEventExtensionShmemSize(), &found);
+ if (found)
+ return;

I think that your error is here. WaitEventExtensionShmemInit() is
forgetting to set the pointer to waitEventExtensionCounter for
processes where IsUnderPostmaster is true, which impacts things not
forked like in -DEXEC_BACKEND (the crash is reproducible on Linux with
-DEXEC_BACKEND in CFLAGS, as well). The correct thing to do is to
always call ShmemInitStruct, but only initialize the contents of the
shared memory area if ShmemInitStruct() has *not* found the shmem
contents.

WaitEventExtensionNew() could be easily incorrectly used, so I'd
rather add a LWLockHeldByMeInMode() on AddinShmemInitLock as safety
measure. Perhaps we should do the same for the LWLocks, subject for a
different thread..

+ int newalloc;
+
+ newalloc = pg_nextpower2_32(Max(8, eventId + 1));

This should be a uint32.

+ if (eventId >= WaitEventExtensionNamesAllocated ||
+ WaitEventExtensionNames[eventId] == NULL)
+ return "extension";
That's too close to the default of "Extension". It would be cleaner
to use "unknown", but we've been using "???" as well in many default
paths where an ID cannot be mapped to a string, so I would recommend
to just use that.

I have spent more time polishing the docs and the comments. This v9
looks in a rather committable shape now with docs, tests and core
routines in place.
--
Michael