Re: Interrupts vs signals

Hi,

On 2025-02-28 22:24:56 +0200, Heikki Linnakangas wrote:
> I noticed that the ShutdownLatchSupport() function is unused. The first
> patch removes it.

Looks like that's the case since

commit 80a8f95b3bca6a80672d1766c928cda34e979112
Author: Andres Freund <andres(at)anarazel(dot)de>
Date: 2021-08-13 05:49:26 -0700

Remove support for background workers without BGWORKER_SHMEM_ACCESS.

Oops.

LGTM.

> The second patch makes it possible to use ModifyWaitEvent() to switch
> between WL_POSTMASTER_DEATH and WL_EXIT_ON_PM_DEATH. WaitLatch() used to
> modify WaitEventSet->exit_on_postmaster_death directly, now it uses
> ModifyWaitEvent() for that. That's needed because with the final patch,
> WaitLatch() is in a different source file than WaitEventSet, so it cannot
> directly modify its field anymore.

> @@ -505,8 +513,14 @@ WaitLatch(Latch *latch, int wakeEvents, long timeout,
> if (!(wakeEvents & WL_LATCH_SET))
> latch = NULL;
> ModifyWaitEvent(LatchWaitSet, LatchWaitSetLatchPos, WL_LATCH_SET, latch);
> - LatchWaitSet->exit_on_postmaster_death =
> - ((wakeEvents & WL_EXIT_ON_PM_DEATH) != 0);
> +
> + /*
> + * Update the event set for whether WL_EXIT_ON_PM_DEATH or
> + * WL_POSTMASTER_DEATH was requested. This is also cheap.
> + */

"for whether" sounds odd to me.

I'd remove the "also" in the second sentence.

> @@ -1037,15 +1051,19 @@ ModifyWaitEvent(WaitEventSet *set, int pos, uint32 events, Latch *latch)
> (!(event->events & WL_LATCH_SET) || set->latch == latch))
> return;
>
> - if (event->events & WL_LATCH_SET &&
> - events != event->events)
> + /* Allow switching between WL_POSTMASTER_DEATH and WL_EXIT_ON_PM_DEATH */
> + if (event->events & WL_POSTMASTER_DEATH)

Hm, this only supports switching from WL_POSTMASTER_DEATH to
WL_EXIT_ON_PM_DEATH, not the other way round, right?

> {
> - elog(ERROR, "cannot modify latch event");
> + if (events != WL_POSTMASTER_DEATH && events != WL_EXIT_ON_PM_DEATH)
> + elog(ERROR, "cannot modify postmaster death event");
> + set->exit_on_postmaster_death = ((events & WL_EXIT_ON_PM_DEATH) != 0);
> + return;
> }

> - if (event->events & WL_POSTMASTER_DEATH)
> + if (event->events & WL_LATCH_SET &&
> + events != event->events)
> {
> - elog(ERROR, "cannot modify postmaster death event");
> + elog(ERROR, "cannot modify latch event");
> }

Why did you reorder this? I don't have a problem with it, I just can't quite
follow.

> The third patch is mechanical and moves existing code. The file header
> comments in the modified files are perhaps worth reviewing. They are also
> just existing text moved around, but there was some small decisions on what
> exactly should go where.
>
> I'll continue working on the other parts, but these patches seems ready for
> commit already.

Interestingly git diff's was pretty annoying to read, even with --color-moved,
unless I used -M100 (setting the rename detection to require 100%
renamed). With "-M100 --color-moved=dimmed-zebra" it looks a lot saner.

> From 7bd0d2f98a1b9d7ad40f3f72cd1c93430c1d7cee Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas <heikki(dot)linnakangas(at)iki(dot)fi>
> Date: Fri, 28 Feb 2025 16:42:15 +0200
> Subject: [PATCH 3/3] Split WaitEventSet functions to separate source file
>
> latch.c now only contains the Latch related functions, which build on
> the WaitEventSet abstraction. Most of the platform-dependent stuff is
> now in waiteventset.c.

> include $(top_srcdir)/src/backend/common.mk
> diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
> index ab601c748f8..aae0cf7577d 100644
> --- a/src/backend/storage/ipc/latch.c
> +++ b/src/backend/storage/ipc/latch.c

> #include "miscadmin.h"

It's a bit weird that MyLatch is declared in miscadmin.h, but that's not this
patch's fault.

> diff --git a/src/backend/storage/ipc/waiteventset.c b/src/backend/storage/ipc/waiteventset.c
> new file mode 100644
> index 00000000000..35e836d3398

> +#include "storage/proc.h"

proc.h wasn't previously included, and it's not clear to me why it'd be needed
now? If I remove it, things still compile, and I verified it's not indirectly
included.

Perhaps you had WakeupMyProc() in proc.c earlier?

> +/*
> + * Change the event mask and, in the WL_LATCH_SET case, the latch associated
> + * with the WaitEvent. The latch may be changed to NULL to disable the latch
> + * temporarily, and then set back to a latch later.
> + *
> + * 'pos' is the id returned by AddWaitEventToSet.
> + */
> +void
> +ModifyWaitEvent(WaitEventSet *set, int pos, uint32 events, Latch *latch)
> +{
> ...
> +
> + /* Allow switching between WL_POSTMASTER_DEATH and WL_EXIT_ON_PM_DEATH */
> + if (event->events & WL_POSTMASTER_DEATH)
> + {
> + if (events != WL_POSTMASTER_DEATH && events != WL_EXIT_ON_PM_DEATH)
> + elog(ERROR, "cannot modify postmaster death event");
> + set->exit_on_postmaster_death = ((events & WL_EXIT_ON_PM_DEATH) != 0);

FWIW, --color-moved flagged this line as having changed. That confused me for
a bit, but it seems that somehow you ended up with a tab after the =. pgindent
wants to remove it too.

> +int
> +WaitEventSetWait(WaitEventSet *set, long timeout,
> + WaitEvent *occurred_events, int nevents,
> + uint32 wait_event_info)
> +{
> ...
> + if (set->latch && !set->latch->is_set)
> + {
> + /* about to sleep on a latch */
> + set->latch->maybe_sleeping = true;
> + pg_memory_barrier();
> + /* and recheck */
> + }

It's a bit ugly that we modify the latch state here - but I don't think the
alternatives are really better...

> +/*
> + * Wake up my process if it's currently waiting on a WaitEventSet.

Hm - that's overstating it a bit, I think. If the WES doesn't wait on the
latch set, this won't wake the process, right?

> +/*
> + * Wake up another process if it's currently waiting.
> + */
> +void
> +WakeupOtherProc(int pid)
> +{
> + kill(pid, SIGURG);
> +}

Dito, I think?

> diff --git a/src/include/storage/latch.h b/src/include/storage/latch.h
> index 66e7a5b7c08..5af32621c0d 100644
> --- a/src/include/storage/latch.h
> +++ b/src/include/storage/latch.h
> @@ -84,10 +84,11 @@
> * use of any generic handler.
> *
> *
> - * WaitEventSets allow to wait for latches being set and additional events -
> - * postmaster dying and socket readiness of several sockets currently - at the
> - * same time. On many platforms using a long lived event set is more
> - * efficient than using WaitLatch or WaitLatchOrSocket.
> + * See also WaitEventSets in waiteventset.h. They allow to wait for latches
> + * being set and additional events - postmaster dying and socket readiness of
> + * several sockets currently - at the same time. On many platforms using a
> + * long lived event set is more efficient than using WaitLatch or
> + * WaitLatchOrSocket.
> *
> *
> * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
> @@ -102,6 +103,7 @@
>
> #include <signal.h>
>
> +#include "storage/waiteventset.h"

Perhaps worth commenting that this is included here for backward compat?

> #include "utils/resowner.h"

I don't think the resowner.h include is still needed in latch.h

> @@ -0,0 +1,95 @@
> +/*-------------------------------------------------------------------------
> + *
> + * waiteventset.h
> + * ppoll() / pselect() like interface for waiting for events
> + *
> + * WaitEventSets allow to wait for latches being set and additional events -
> + * postmaster dying and socket readiness of several sockets currently - at the
> + * same time. On many platforms using a long lived event set is more
> + * efficient than using WaitInterrupt or WaitInterruptOrSocket.

I don't think WaitInterrupt/WaitInterruptOrSocket exist at this stage?

These minor things aside, this looks good to me.

Greetings,

Andres Freund