Re: Interrupts vs signals - Mailing list pgsql-hackers

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@anarazel.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.linnakangas@iki.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