Re: Support to define custom wait events for extensions

On Wed, Aug 09, 2023 at 08:10:42PM +0900, Masahiro Ikeda wrote:
> * create second hash table to find a event id from a name to
> identify uniquness. It enable extensions which don't use share
> memory for their use to define custom wait events because
> WaitEventExtensionNew() will not allocate duplicate wait events.

Okay, a second hash table to check if events are registered works for
me.

> * create PoC patch to show that extensions, which don't use shared
> memory for their use, can define custom wait events.
> (v2-0002-poc-custom-wait-event-for-dblink.patch)
>
> I'm worrying about
> * Is 512(wee_hash_max_size) the maximum number of the custom wait
> events sufficient?

Thanks for sending a patch!

I'm OK to start with that. This could always be revisited later, but
even for a server loaded with a bunch of extensions that looks more
than enough to me.

> * Is there any way to not force extensions that don't use shared
> memory for their use like dblink to acquire AddinShmemInitLock?;

Yes, they don't need it at all as the dynahashes are protected with
their own LWLocks.

+++ b/src/backend/storage/lmgr/lwlocknames.txt
@@ -53,3 +53,4 @@ XactTruncationLock 44
# 45 was XactTruncationLock until removal of BackendRandomLock
WrapLimitsVacuumLock 46
NotifyQueueTailLock 47
+WaitEventExtensionLock 48

This new LWLock needs to be added to wait_event_names.txt, or it won't
be reported to pg_stat_activity and it would not be documented when
the sgml docs are generated from the txt data.

-extern uint32 WaitEventExtensionNew(void);
-extern void WaitEventExtensionRegisterName(uint32 wait_event_info,
- const char *wait_event_name);
+extern uint32 WaitEventExtensionNew(const char *wait_event_name);
Looks about right, and the docs are refreshed.

+static const int wee_hash_init_size = 128;
+static const int wee_hash_max_size = 512;
I would use a few #defines with upper-case characters here instead as
these are constants for us.

Now that it is possible to rely on LWLocks for the hash tables, more
cleanup is possible in worker_spi, with the removal of
worker_spi_state, the shmem hooks and their routines. The only thing
that should be needed is something like that at the start of
worker_spi_main() (same position as worker_spi_shmem_init now):
+static uint32 wait_event = 0;
[...]
+ if (wait_event == 0)
+ wait_event = WaitEventExtensionNew("worker_spi_main");

The updates in 001_worker_spi.pl look OK.

+ * The entry must be stored because it's registered in
+ * WaitEventExtensionNew().
*/
- eventId -= NUM_BUILTIN_WAIT_EVENT_EXTENSION;
+ if (!entry)
+ ereport(ERROR,
+ errmsg("could not find the name for custom wait event ID %u", eventId));
Yeah, I think that's better than just falling back to "extension". An
ID reported in pg_stat_activity should always have an entry, or we
have race conditions. This should be an elog(ERROR), as in
this-error-shall-never-happen. No need to translate the error string,
as well (the docs have been updated with this change. thanks!).

Additionally, LWLockHeldByMeInMode(AddinShmemInitLock) in
WaitEventExtensionNew() should not be needed, thanks to
WaitEventExtensionLock.

+ * WaitEventExtensionNameHash is used to find the name from a event id.
+ * It enables all backends look up them without additional processing
+ * per backend like LWLockRegisterTranche().

It does not seem necessary to mention LWLockRegisterTranche().

+ * WaitEventExtensionIdHash is used to find the event id from a name.
+ * Since it can identify uniquness by the names, extensions that do not
+ * use shared memory also be able to define custom wait events without
+ * defining duplicate wait events.

Perhaps this could just say that this table is necessary to ensure
that we don't have duplicated entries when registering new strings
with their IDs? s/uniquness/uniqueness/. The second part of the
sentence about shared memory does not seem necessary now.

+ sz = add_size(sz, hash_estimate_size(wee_hash_init_size,
+ sizeof(WaitEventExtensionNameEntry)));
+ sz = add_size(sz, hash_estimate_size(wee_hash_init_size,
+ sizeof(WaitEventExtensionIdEntry)));

Err, this should use the max size, and not the init size for the size
estimation, no?

+ if (strlen(wait_event_name) >= NAMEDATALEN)
+ ereport(ERROR,
+ errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("wait event name is too long"));
This could just be an elog(ERROR), I assume, as that could only be
reached by developers. The string needs to be rewritten, like "cannot
use custom wait event string longer than %u characters", or something
like that.

+ if (wait_event_info == NULL)
+ {
+ wait_event_info = (uint32 *) MemoryContextAlloc(TopMemoryContext, sizeof(uint32));
+ LWLockAcquire(AddinShmemInitLock, LW_EXCLUSIVE);
+ *wait_event_info = WaitEventExtensionNew("dblink_get_con");
+ LWLockRelease(AddinShmemInitLock);
+ }
+ conn = libpqsrv_connect(connstr, *wait_event_info)

In 0002. Caching the value statically in the backend is what you
should do, but a pointer, an allocation to the TopMemoryContext and a
dependency to AddinShmemInitLock should not be necessary when dealing
with a uint32. You could use an initial value of 0, for example, or
just PG_WAIT_EXTENSION but the latter is not really necessary and
would bypass the sanity checks.

+ /* Register the new custom wait event in the shared hash table */
+ LWLockAcquire(WaitEventExtensionLock, LW_EXCLUSIVE);
+
+ name_entry = (WaitEventExtensionNameEntry *)
+ hash_search(WaitEventExtensionNameHash, &eventId, HASH_ENTER, &found);
+ Assert(!found);
+ strlcpy(name_entry->wait_event_name, wait_event_name, sizeof(name_entry->wait_event_name));
+
+ id_entry = (WaitEventExtensionIdEntry *)
+ hash_search(WaitEventExtensionIdHash, &wait_event_name, HASH_ENTER, &found);
+ Assert(!found);
+ id_entry->event_id = eventId;
+
+ LWLockRelease(WaitEventExtensionLock);

The logic added to WaitEventExtensionNew() is a bit racy, where it
would be possible with the same entry to be added multiple times.
Imagine for example the following:
- Process 1 does WaitEventExtensionNew("foo1"), does not find the
entry by name in hash_search, gets an eventId of 1, releases the
spinlock.
- Process 2 calls as well WaitEventExtensionNew("foo1"), does not find
the entry by name because it has not been added by process 1 yet,
allocates an eventId of 2
- Process 2 takes first WaitEventExtensionLock in LW_EXCLUSIVE to add
entry "foo1", there is no entry by name, so one is added for the ID.
WaitEventExtensionLock is released
- Process 1, that was waiting on WaitEventExtensionLock, can now take
it in exclusive mode. It finds an entry by name for "foo1", fails the
assertion because an entry is found.

I think that the ordering of WaitEventExtensionNew() should be
reworked a bit. This order should be safer.
- Take WaitEventExtensionLock in shared mode, look if there's an entry
by name, release the lock. The patch does that.
- If an entry is found, return, we're OK. The patch does that.
- Take again WaitEventExtensionLock in exclusive mode.
- Look again at the hash table with the name given, in case somebody
has inserted an equivalent entry in the short window where the lock
was not held.
-- If an entry is found, release the lock and leave, we're OK.
-- If an entry is not found, keep the lock.
- Acquire the spinlock, and get a new event ID. Release spinlock.
- Add the new entries to both tables, both assertions on found are OK
to have.
- Release LWLock and leave.
--
Michael