Refine comments on usage WL_POSTMASTER_DEATH vs WL_EXIT_ON_PM_DEATH
Hi, Hackers!
Current comments on the usage of WL_POSTMASTER_DEATH state that it
should be used for scenarios of finishing other than immediately i.e.
returning values and waiting for postmaster dies.
In fact, in parts of the code, it's currently used to immediately exit or
throw FATAL (in the walsender and in libpq).
So I propose change the comments on WL_POSTMASTER_DEATH stating that it
could be used for both cases: for processing and setting return values if
that's needed, and for immediate exit otherwise.
Regards,
Pavel Borisov
Supabase
Attachments:
v1-0001-Refine-comments-on-usage-WL_POSTMASTER_DEATH.patchapplication/octet-stream; name=v1-0001-Refine-comments-on-usage-WL_POSTMASTER_DEATH.patchDownload
From 1e50f3501a415068242b4cf1f28f82f7a39054f8 Mon Sep 17 00:00:00 2001
From: Pavel Borisov <pashkin.elfe@gmail.com>
Date: Wed, 23 Oct 2024 13:06:44 +0400
Subject: [PATCH v1] Refine comments on usage WL_POSTMASTER_DEATH
---
src/backend/storage/ipc/latch.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
index 608eb66abe..218607d7bd 100644
--- a/src/backend/storage/ipc/latch.c
+++ b/src/backend/storage/ipc/latch.c
@@ -554,8 +554,10 @@ WaitLatch(Latch *latch, int wakeEvents, long timeout,
*
* wakeEvents must include either WL_EXIT_ON_PM_DEATH for automatic exit
* if the postmaster dies or WL_POSTMASTER_DEATH for a flag set in the
- * return value if the postmaster dies. The latter is useful for rare cases
- * where some behavior other than immediate exit is needed.
+ * return value if the postmaster dies. The latter is useful for cases
+ * where some behavior other than immediate exit could be needed, though
+ * immediate exit is also allowed in processing WL_POSTMASTER_DEATH if
+ * no other behavior needed.
*
* NB: These days this is just a wrapper around the WaitEventSet API. When
* using a latch very frequently, consider creating a longer living
@@ -693,7 +695,7 @@ SetLatch(Latch *latch)
}
else
kill(owner_pid, SIGURG);
-
+/:
#else
/*
@@ -930,7 +932,7 @@ FreeWaitEventSetAfterFork(WaitEventSet *set)
/* ---
* Add an event to the set. Possible events are:
* - WL_LATCH_SET: Wait for the latch to be set
- * - WL_POSTMASTER_DEATH: Wait for postmaster to die
+ * - WL_POSTMASTER_DEATH: Wait for postmaster to die or finish immediately
* - WL_SOCKET_READABLE: Wait for socket to become readable,
* can be combined in one event with other WL_SOCKET_* events
* - WL_SOCKET_WRITEABLE: Wait for socket to become writeable,
@@ -958,6 +960,10 @@ FreeWaitEventSetAfterFork(WaitEventSet *set)
* The user_data pointer specified here will be set for the events returned
* by WaitEventSetWait(), allowing to easily associate additional data with
* events.
+ *
+ * Unlike WL_EXIT_ON_PM_DEATH that is only for immediate exit WL_POSTMASTER_DEATH
+ * allows the process to set a return values and wait for postmaster to die, but
+ * also allows exiting immediately if return value is not needed.
*/
int
AddWaitEventToSet(WaitEventSet *set, uint32 events, pgsocket fd, Latch *latch,
--
2.39.2 (Apple Git-143)
On 23/10/2024 12:18, Pavel Borisov wrote:
Hi, Hackers!
Current comments on the usage of WL_POSTMASTER_DEATH state that it
should be used for scenarios of finishing other than immediately i.e.
returning values and waiting for postmaster dies.
In fact, in parts of the code, it's currently used to immediately exit
or throw FATAL (in the walsender and in libpq).So I propose change the comments on WL_POSTMASTER_DEATH stating that it
could be used for both cases: for processing and setting return values
if that's needed, and for immediate exit otherwise.
I see what you mean, but I don't think the proposed patch is making it
better. With WL_POSTMASTER_DEATH, the WaitLatch call returns if the
postmaster dies. What the caller does then is the caller's business.
That's different from WL_EXIT_ON_PM_DEATH in that with
WL_EXIT_ON_PM_DEATH, WaitLatch itself will do the exit(), not the caller.
--
Heikki Linnakangas
Neon (https://neon.tech)
Hi, Heikki!
On Wed, 23 Oct 2024 at 21:00, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 23/10/2024 12:18, Pavel Borisov wrote:
Hi, Hackers!
Current comments on the usage of WL_POSTMASTER_DEATH state that it
should be used for scenarios of finishing other than immediately i.e.
returning values and waiting for postmaster dies.
In fact, in parts of the code, it's currently used to immediately exit
or throw FATAL (in the walsender and in libpq).So I propose change the comments on WL_POSTMASTER_DEATH stating that it
could be used for both cases: for processing and setting return values
if that's needed, and for immediate exit otherwise.I see what you mean, but I don't think the proposed patch is making it
better. With WL_POSTMASTER_DEATH, the WaitLatch call returns if the
postmaster dies. What the caller does then is the caller's business.
That's different from WL_EXIT_ON_PM_DEATH in that with
WL_EXIT_ON_PM_DEATH, WaitLatch itself will do the exit(), not the caller.
That was exactly my point. Actually the caller should not wait, it could do
whatever it wants contrary to the existing comments:
WL_POSTMASTER_DEATH: Wait for postmaster to die
I don't insist on this patch, but existing comments on this look somewhat
misleading.
Regards,
Pavel Borisov
On 23/10/2024 20:29, Pavel Borisov wrote:
Hi, Heikki!
On Wed, 23 Oct 2024 at 21:00, Heikki Linnakangas <hlinnaka@iki.fi
<mailto:hlinnaka@iki.fi>> wrote:On 23/10/2024 12:18, Pavel Borisov wrote:
Hi, Hackers!
Current comments on the usage of WL_POSTMASTER_DEATH state that it
should be used for scenarios of finishing other than immediatelyi.e.
returning values and waiting for postmaster dies.
In fact, in parts of the code, it's currently used to immediatelyexit
or throw FATAL (in the walsender and in libpq).
So I propose change the comments on WL_POSTMASTER_DEATH stating
that it
could be used for both cases: for processing and setting return
values
if that's needed, and for immediate exit otherwise.
I see what you mean, but I don't think the proposed patch is making it
better. With WL_POSTMASTER_DEATH, the WaitLatch call returns if the
postmaster dies. What the caller does then is the caller's business.
That's different from WL_EXIT_ON_PM_DEATH in that with
WL_EXIT_ON_PM_DEATH, WaitLatch itself will do the exit(), not the
caller.That was exactly my point. Actually the caller should not wait, it could
do whatever it wants contrary to the existing comments:WL_POSTMASTER_DEATH: Wait for postmaster to die
I don't insist on this patch, but existing comments on this look
somewhat misleading.
Ok I seem to totally not understand what the problem is then. The
comment seems fine to me. ¯\_(ツ)_/¯
--
Heikki Linnakangas
Neon (https://neon.tech)
Heikki Linnakangas <hlinnaka@iki.fi> writes:
On 23/10/2024 20:29, Pavel Borisov wrote:
That was exactly my point. Actually the caller should not wait, it could
do whatever it wants contrary to the existing comments:WL_POSTMASTER_DEATH: Wait for postmaster to die
I don't insist on this patch, but existing comments on this look
somewhat misleading.
Ok I seem to totally not understand what the problem is then. The
comment seems fine to me. ¯\_(ツ)_/¯
Yeah, me too. The "Wait for ..." wording is exactly like all the
other events in the list, as it should be since the semantics are
the same. Maybe we could write "Stop waiting when ..." but
is that really much clearer?
I suspect the documentation that Pavel is missing is
* wakeEvents must include either WL_EXIT_ON_PM_DEATH for automatic exit
* if the postmaster dies or WL_POSTMASTER_DEATH for a flag set in the
* return value if the postmaster dies. The latter is useful for rare cases
* where some behavior other than immediate exit is needed.
which for some reason is down in the header for WaitLatchOrSocket,
not close to the list at AddWaitEventToSet. And really *both* of
those comment blocks are in the wrong place. I propose moving them
to be in front of the #defines for the WL_XXX symbols in latch.h.
But I don't feel a need to re-word those comments.
regards, tom lane
On Wednesday, October 23, 2024, Pavel Borisov <pashkin.elfe@gmail.com>
wrote:
In fact, in parts of the code, it's currently used to immediately exit or
throw FATAL (in the walsender and in libpq).
Maybe submit a patch to fix these parts of the code that should apparently
be using WL_EXIT_ON_PM_DEATH to do so instead of an unnecessary
self-directed exit?
The comment needn’t tell the developer that it is possible code things in
such a way using WL_POSTMASTER_DEATH that is redundant with what they could
more easily accomplish using WL_EXIT_ON_PM_DEATH. Doing nothing is always
an option, but one not recommended and thus not pointed out.
The only thing that I could see not leaving implied is that if you don’t
use WL_EXIT_ON_PM_DEATH and the postmaster does die your handler code must
terminate your process explicitly (I suppose “must” really is “should”
because I suspect when the postmaster is done dead it will have taken its
children to /dev/null along with it…).
David J.