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