Thread: Re: Refine comments on usage WL_POSTMASTER_DEATH vs WL_EXIT_ON_PM_DEATH
Re: Refine comments on usage WL_POSTMASTER_DEATH vs WL_EXIT_ON_PM_DEATH
From
Heikki Linnakangas
Date:
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
Re: Refine comments on usage WL_POSTMASTER_DEATH vs WL_EXIT_ON_PM_DEATH
From
Heikki Linnakangas
Date:
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 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. 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