Re: Support to define custom wait events for extensions

On Thu, Aug 10, 2023 at 01:08:39PM +0900, Masahiro Ikeda wrote:
> In addition, I change the followings:
> * update about custom wait events in sgml. we don't need to use
> shmem_startup_hook.
> * change the hash names for readability.
> (ex. WaitEventExtensionNameHash -> WaitEventExtensionHashById)
> * fix a bug to fail to get already defined events by names
> because HASH_BLOBS was specified. HASH_STRINGS is right since
> the key is C strings.

That's what I get based on what ShmemInitHash() relies on.

I got a few more comments about v3. Overall this looks much better.

This time the ordering of the operations in WaitEventExtensionNew()
looks much better.

+ * The entry must be stored because it's registered in
+ * WaitEventExtensionNew().
Not sure of the value added by this comment, I would remove it.

+ if (!entry)
+ elog(ERROR, "could not find the name for custom wait event ID %u",
+ eventId);

Or a simpler "could not find custom wait event name for ID %u"?

+static HTAB *WaitEventExtensionHashById; /* find names from ids */
+static HTAB *WaitEventExtensionHashByName; /* find ids from names */

These names are OK here.

+/* Local variables */
+static uint32 worker_spi_wait_event = 0;
That's a cached value, used as a placeholder for the custom wait event
ID found from the table.

+ HASH_ELEM | HASH_STRINGS); /* key is Null-terminated C strings */
Looks obvious to me based on the code, I would remove this note.

+/* hash table entres */
s/entres/entries/

+ /*
+ * Allocate and register a new wait event. But, we need to recheck because
+ * other processes could already do so while releasing the lock.
+ */

Could be reworked for the second sentence, like "Recheck if the event
exists, as it could be possible that a concurrent process has inserted
one with the same name while the lock was previously released."

+ /* Recheck */
Duplicate.

/* OK to make connection */
- conn = libpqsrv_connect(connstr, WAIT_EVENT_EXTENSION);
+ if (wait_event_info == 0)
+ wait_event_info = WaitEventExtensionNew("dblink_get_con");
+ conn = libpqsrv_connect(connstr, wait_event_info);

It is going to be difficult to make that simpler ;)

This looks correct, but perhaps we need to think harder about the
custom event names and define a convention when more of this stuff is
added to the core modules.
--
Michael