Обсуждение: Rethink the wait event names for postgres_fdw, dblink and etc

Hi,

Recently, the API to define custom wait events for extension is 
supported.
* Change custom wait events to use dynamic shared hash tables(af720b4c5)

So, I'd like to rethink the wait event names for modules which use
"WAIT_EVENT_EXTENSION" wait events.
* postgres_fdw
* dblink
* pg_prewarm
* test_shm_mq
* worker_spi

I expect that no one will object to changing the names to appropriate
ones. But, we need to discuss that naming convention, the names 
themselves,
document descriptions and so on.

I made the v1 patch
* CamelCase naming convention
* Add document descriptions for each module

I haven't added document descriptions for pg_prewarm and test modules.
The reason is that the wait event of autoprewarm is not shown on
pg_stat_activity. It's not an auxiliary-process and doesn't connect to
a database, so pgstat_bestart() isn't be called.

Feedback is always welcome and appreciated.

Regards,
-- 
Masahiro Ikeda
NTT DATA CORPORATION

On Fri, Aug 18, 2023 at 12:27:02PM +0900, Masahiro Ikeda wrote:
> I expect that no one will object to changing the names to appropriate
> ones. But, we need to discuss that naming convention, the names themselves,
> document descriptions and so on.
>
> I made the v1 patch
> * CamelCase naming convention

Not sure how others feel about that, but I am OK with camel-case style
for the modules.

> I haven't added document descriptions for pg_prewarm and test modules.
> The reason is that the wait event of autoprewarm is not shown on
> pg_stat_activity. It's not an auxiliary-process and doesn't connect to
> a database, so pgstat_bestart() isn't called.

Perhaps we could just leave it out, then, adding a comment instead.

> Feedback is always welcome and appreciated.

+        /* first time, allocate or get the custom wait event */
+        if (wait_event_info == 0)
+            wait_event_info = WaitEventExtensionNew("DblinkConnect");
[...]
+    /* first time, allocate or get the custom wait event */
+    if (wait_event_info == 0)
+        wait_event_info = WaitEventExtensionNew("DblinkConnect");

Shouldn't dblink use two different strings?

+       if (wait_event_info == 0)
+           wait_event_info = WaitEventExtensionNew("PgPrewarmDumpDelay");

Same about autoprewarm.c.  The same flag is used in two different code
paths.  If removed from the patch, no need to do that, of course.

+static uint32 wait_event_info_connect = 0;
+static uint32 wait_event_info_receive = 0;
+static uint32 wait_event_info_cleanup_receive = 0;

Perhaps such variables could be named with shorter names proper to
each module, like pgfdw_we_receive, etc.

+   <filename>dblink</filename> could show the following wait event under the wait

s/could show/can report/?

+      Waiting for same reason as <literal>PostgresFdwReceive</literal>, except that it's only for
+      abort.

"Waiting for transaction abort on remote server"?
---
Michael

Hi,

Thanks for your comments.

I updated the patch to v2.
* Update a comment instead writing documentation about
   the wait events for pg_prewarm.
* Make the name of wait events which are different code
   path different. Add DblinkGetConnect and PgPrewarmDumpShutdown.
* Make variable names shorter like pgfdw_we_receive.
* Update documents.
* Add some tests with pg_wait_events view.

Regards,
-- 
Masahiro Ikeda
NTT DATA CORPORATION

On Mon, Aug 21, 2023 at 11:04:23AM +0900, Masahiro Ikeda wrote:
> I updated the patch to v2.
> * Update a comment instead writing documentation about
>   the wait events for pg_prewarm.

Right.  It does not seem worth the addition currently, so I am
discarded this part.  It's just not worth the extra cycles for the
moment.

> * Make the name of wait events which are different code
>   path different. Add DblinkGetConnect and PgPrewarmDumpShutdown.
> * Make variable names shorter like pgfdw_we_receive.
> * Update documents.
> * Add some tests with pg_wait_events view.

Sounds like a good idea for postgres_fdw and dblink, still some of
them may not be stable?  First, PostgresFdwReceive and
PostgresFdwCleanupReceive would be registered only if the connection
is busy, but that may not be always the case depending on the timing?
PostgresFdwConnect is always OK because this code path in
connect_pg_server() is always taken.  Similarly, DblinkConnect and
DblinkGetConnect are registered in deterministic code paths, so these
will show up all the time.

I am lacking a bit of time now, but I have applied the bits for
test_shm_mq and worker_spi.  Note that I have not added tests for
test_shm_mq as it may be possible that the two events (for the
bgworker startup and for a message to be queued) are never reached
depending on the timing.  I'll handle the rest tomorrow, with likely
some adjustments to the tests.  (I may as well just remove them, this
API is already covered by worker_spi.)
--
Michael

On Wed, Oct 04, 2023 at 05:19:40PM +0900, Michael Paquier wrote:
> I am lacking a bit of time now, but I have applied the bits for
> test_shm_mq and worker_spi.  Note that I have not added tests for
> test_shm_mq as it may be possible that the two events (for the
> bgworker startup and for a message to be queued) are never reached
> depending on the timing.  I'll handle the rest tomorrow, with likely
> some adjustments to the tests.  (I may as well just remove them, this
> API is already covered by worker_spi.)

After sleeping on it, I've taken the decision to remove the tests.  As
far as I have tested, this was stable, but this does not really
improve the test coverage as WaitEventExtensionNew() is covered in
worker_spi.  I have done tweaks to the docs and the variable names,
and applied that into its own commit.

Note as well that the docs of dblink were wrong for DblinkGetConnect:
the wait event could be seen in other functions than dblink() and
dblink_exec().
--
Michael

On 2023-10-05 10:28, Michael Paquier wrote:
> On Wed, Oct 04, 2023 at 05:19:40PM +0900, Michael Paquier wrote:
>> I am lacking a bit of time now, but I have applied the bits for
>> test_shm_mq and worker_spi.  Note that I have not added tests for
>> test_shm_mq as it may be possible that the two events (for the
>> bgworker startup and for a message to be queued) are never reached
>> depending on the timing.  I'll handle the rest tomorrow, with likely
>> some adjustments to the tests.  (I may as well just remove them, this
>> API is already covered by worker_spi.)
> 
> After sleeping on it, I've taken the decision to remove the tests.  As
> far as I have tested, this was stable, but this does not really
> improve the test coverage as WaitEventExtensionNew() is covered in
> worker_spi.  I have done tweaks to the docs and the variable names,
> and applied that into its own commit.
> 
> Note as well that the docs of dblink were wrong for DblinkGetConnect:
> the wait event could be seen in other functions than dblink() and
> dblink_exec().

Thanks for modifying and committing. I agree your comments.

Regards,
-- 
Masahiro Ikeda
NTT DATA CORPORATION