On 2023-07-12 09:36, Andres Freund wrote:
Hi,
On 2023-07-11 16:45:26 +0900, Michael Paquier wrote:
+/* ----------
+ * Wait Events - Extension
+ *
+ * Use this category when the server process is waiting for some
condition
+ * defined by an extension module.
+ *
+ * Extensions can define custom wait events. First, call
+ * WaitEventExtensionNewTranche() just once to obtain a new wait
event
+ * tranche. The ID is allocated from a shared counter. Next, each
+ * individual process using the tranche should call
+ * WaitEventExtensionRegisterTranche() to associate that wait event
with
+ * a name.
What does "tranche" mean here? For LWLocks it makes some sense, it's
used for
a set of lwlocks, not an individual one. But here that doesn't really
seem to
apply?
Thanks for useful comments.
OK, I will change to WaitEventExtensionNewId() and
WaitEventExtensionRegisterName().
+ * It may seem strange that each process using the tranche must
register it
+ * separately, but dynamic shared memory segments aren't guaranteed
to be
+ * mapped at the same address in all coordinating backends, so
storing the
+ * registration in the main shared memory segment wouldn't work for
that case.
+ */
I don't really see how this applies to wait events? There's no pointers
here...
Yes, I'll fix the comments.
+typedef enum
+{
+ WAIT_EVENT_EXTENSION = PG_WAIT_EXTENSION,
+ WAIT_EVENT_EXTENSION_FIRST_USER_DEFINED
+} WaitEventExtension;
+
+extern void WaitEventExtensionShmemInit(void);
+extern Size WaitEventExtensionShmemSize(void);
+
+extern uint32 WaitEventExtensionNewTranche(void);
+extern void WaitEventExtensionRegisterTranche(uint32 wait_event_info,
-slock_t *ShmemLock; /* spinlock for shared memory and LWLock
+slock_t *ShmemLock; /* spinlock for shared memory, LWLock
+ * allocation,
and named extension wait event
* allocation */
I'm doubtful that it's a good idea to reuse the spinlock further. Given
that
the patch adds WaitEventExtensionShmemInit(), why not just have a lock
in
there?
OK, I'll create a new spinlock for the purpose.
+/*
+ * Allocate a new event ID and return the wait event info.
+ */
+uint32
+WaitEventExtensionNewTranche(void)
+{
+ uint16 eventId;
+
+ SpinLockAcquire(ShmemLock);
+ eventId = (*WaitEventExtensionCounter)++;
+ SpinLockRelease(ShmemLock);
+
+ return PG_WAIT_EXTENSION | eventId;
+}
It seems quite possible to run out space in WaitEventExtensionCounter,
so this
should error out in that case.
OK, I'll do so.
+/*
+ * Register a dynamic tranche name in the lookup table of the current
process.
+ *
+ * This routine will save a pointer to the wait event tranche name
passed
+ * as an argument, so the name should be allocated in a
backend-lifetime context
+ * (shared memory, TopMemoryContext, static constant, or similar).
+ *
+ * The "wait_event_name" will be user-visible as a wait event name,
so try to
+ * use a name that fits the style for those.
+ */
+void
+WaitEventExtensionRegisterTranche(uint32 wait_event_info,
+ const char
*wait_event_name)
+{
+ uint16 eventId;
+
+ /* Check wait event class. */
+ Assert((wait_event_info & 0xFF000000) == PG_WAIT_EXTENSION);
+
+ eventId = wait_event_info & 0x0000FFFF;
+
+ /* This should only be called for user-defined tranches. */
+ if (eventId < NUM_BUILTIN_WAIT_EVENT_EXTENSION)
+ return;
Why not assert out in that case then?
OK, I'll add the assertion for eventID.
+/*
+ * Return the name of an Extension wait event ID.
+ */
+static const char *
+GetWaitEventExtensionIdentifier(uint16 eventId)
+{
+ /* Build-in tranche? */
*built
I will fix it.
+ if (eventId < NUM_BUILTIN_WAIT_EVENT_EXTENSION)
+ return "Extension";
+
+ /*
+ * It's an extension tranche, so look in
WaitEventExtensionTrancheNames[].
+ * However, it's possible that the tranche has never been registered
by
+ * calling WaitEventExtensionRegisterTranche() in the current
process, in
+ * which case give up and return "Extension".
+ */
+ eventId -= NUM_BUILTIN_WAIT_EVENT_EXTENSION;
+
+ if (eventId >= WaitEventExtensionTrancheNamesAllocated ||
+ WaitEventExtensionTrancheNames[eventId] == NULL)
+ return "Extension";
I'd return something different here, otherwise something that's
effectively a
bug is not distinguishable from the built
It is a good idea. It would be good to name it "unknown wait event", the
same as
pgstat_get_wait_activity(), etc.
+++ b/src/test/modules/test_custom_wait_events/t/001_basic.pl
@@ -0,0 +1,34 @@
+# Copyright (c) 2023, PostgreSQL Global Development Group
+
+use strict;
+use warnings;
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node = PostgreSQL::Test::Cluster->new('main');
+
+$node->init;
+$node->append_conf(
+ 'postgresql.conf',
+ "shared_preload_libraries = 'test_custom_wait_events'"
+);
+$node->start;
I think this should also test registering a wait event later.
I see. I wasn't expecting that.
@@ -0,0 +1,188 @@
+/*--------------------------------------------------------------------------
+ *
+ * test_custom_wait_events.c
+ * Code for testing custom wait events
+ *
+ * This code initializes a custom wait event in shmem_request_hook()
and
+ * provide a function to launch a background worker waiting forever
+ * with the custom wait event.
Isn't this vast overkill? Why not just have a simple C function that
waits
with a custom wait event, until cancelled? That'd maybe 1/10th of the
LOC.
Are you saying that processing in the background worker is overkill?
If my understanding is correct, it is difficult to implement the test
without a background worker for this purpose. This is because the test
will execute commands serially, while a function waiting is executed in
a backend process, it is not possible to connect to another backend and
check the wait events on pg_stat_activity view.
Please let me know if my understanding is wrong.
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
