Message-ID: <46f047a1-bfd5-406a-a0f3-97205d9529ad@iki.fi>
Date: Wed, 7 Aug 2024 17:59:23 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
From: Heikki Linnakangas <hlinnaka@iki.fi>
Subject: Re: Interrupts vs signals
To: Thomas Munro <thomas.munro@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>,
 Andres Freund <andres@anarazel.de>, Robert Haas <robertmhaas@gmail.com>
References: <CA+hUKG+3MkS21yK4jL4cgZywdnnGKiBg0jatoV6kzaniBmcqbQ@mail.gmail.com>
 <20211020192748.dguzfnnd4hzh7e5v@alap3.anarazel.de>
 <CA+hUKGLseL7e7EFMZZECzCijjov76PcXEGYy2swXa8jfnuV=2A@mail.gmail.com>
 <CA+TgmobVnFHGww-7z6vC1Ht63Pj-Kus4dFYPOp2zgMY2M6_iDw@mail.gmail.com>
 <20211111195000.v5fmy5yvtqcbvo55@alap3.anarazel.de>
 <CA+TgmobfJHdTjh8sDGct_bP0wfB0J6Csz7OHOEbiEu62ZD5jGA@mail.gmail.com>
 <CA+hUKGJACzP++3PifHTMgG2xgTD4irap6+CXBMogKKiD43VqoA@mail.gmail.com>
 <CA+hUKGLAZCxzyhyoYz0BmGkqkmj-38ZfToF1GqNYhU+Br+XCLQ@mail.gmail.com>
 <3b996d73-2a10-4072-a58d-0386af0cbb78@iki.fi>
 <CA+hUKGJvt8wg_KfjFLvr27LT6sYK_szngkchig6Dg_CnNGPskw@mail.gmail.com>
Content-Language: en-US
In-Reply-To: <CA+hUKGJvt8wg_KfjFLvr27LT6sYK_szngkchig6Dg_CnNGPskw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/46f047a1-bfd5-406a-a0f3-97205d9529ad%40iki.fi>
Precedence: bulk

On 10/07/2024 09:48, Thomas Munro wrote:
> The next problems to remove are, I think, the various SIGUSR2, SIGINT,
> SIGTERM signals sent by the postmaster.  These should clearly become
> SendInterrupt() or ProcSetLatch().  The problem here is that the
> postmaster doesn't have the proc numbers yet.  One idea is to teach
> the postmaster to assign them!  Not explored yet.

With my latest patches on the "Refactoring postmaster's code to cleanup 
after child exit" thread [1], every postmaster child process is assigned 
a slot in the pmsignal.c array, including all the aux processes. If we 
moved 'pending_interrupts' and the process Latch to the pmsignal.c 
array, then you could send an interrupt also to a process that doesn't 
have a PGPROC entry. That includes dead-end backends, backends that are 
still in authentication, and the syslogger.

That would also make it so that the postmaster would never need to poke 
into the procarray. pmsignal.c is already designated as the limited 
piece of shared memory that is accessed by the postmaster 
(BackgroundWorkerSlots is the other exception), so it would be kind of 
nice if all the information that the postmaster needs to send an 
interrupt was there. That would mean that where you currently use a 
ProcNumber to identify a process, you'd use an index into the 
PMSignalState array instead.

I don't insist on changing that right now, I think this patch is OK as 
it is, but that might be a good next step later.

[1] 
https://www.postgresql.org/message-id/8f2118b9-79e3-4af7-b2c9-bd5818193ca4%40iki.fi

I'm also wondering about the relationship between interrupts and 
latches. Currently, SendInterrupt sets a latch to wake up the target 
process. I wonder if it should be the other way 'round? Move all the 
wakeup code, with the signalfd, the self-pipe etc to interrupt.c, and in 
SetLatch, call SendInterrupt to wake up the target process? Somehow that 
feels more natural to me, I think.

> This version is passing on Windows.  I'll create a CF entry.  Still
> work in progress!

Some comments on the patch details:

>  		ereport(WARNING,
>  				(errmsg("NOTIFY queue is %.0f%% full", fillDegree * 100),
> -				 (minPid != InvalidPid ?
> -				  errdetail("The server process with PID %d is among those with 
the oldest transactions.", minPid)
> +				 (minPgprocno != INVALID_PROC_NUMBER ?
> +				  errdetail("The server process with pgprocno %d is among those 
with the oldest transactions.", minPgprocno)
>  				  : 0),
> -				 (minPid != InvalidPid ?
> +				 (minPgprocno != INVALID_PROC_NUMBER ?
>  				  errhint("The NOTIFY queue cannot be emptied until that process 
ends its current transaction.")
>  				  : 0)));

This makes the message less useful to the user, as the ProcNumber isn't 
exposed to users. With the PID, you can do "pg_terminate_backend(pid)"

> diff --git a/src/backend/optimizer/util/pathnode.c 
b/src/backend/optimizer/util/pathnode.c
> index c42742d2c7b..bfb89049020 100644
> --- a/src/backend/optimizer/util/pathnode.c
> +++ b/src/backend/optimizer/util/pathnode.c
> @@ -18,6 +18,7 @@
>
>  #include "foreign/fdwapi.h"
>  #include "miscadmin.h"
> +#include "postmaster/interrupt.h"
>  #include "nodes/extensible.h"
>  #include "optimizer/appendinfo.h"
>  #include "optimizer/clauses.h"

misordered

> +	 * duplicated interrupts later if we switch backx.

typo: backx -> back

> -	if (IdleInTransactionSessionTimeoutPending)
> +	if (ConsumeInterrupt(INTERRUPT_IDLE_TRANSACTION_TIMEOUT))
>  	{
>  		/*
>  		 * If the GUC has been reset to zero, ignore the signal.  This is
> @@ -3412,7 +3361,6 @@ ProcessInterrupts(void)
>  		 * interrupt.  We need to unset the flag before the injection point,
>  		 * otherwise we could loop in interrupts checking.
>  		 */
> -		IdleInTransactionSessionTimeoutPending = false;
>  		if (IdleInTransactionSessionTimeout > 0)
>  		{
>  			INJECTION_POINT("idle-in-transaction-session-timeout");

The "We need to unset the flag.." comment is a bit out of place now, 
since the flag was already cleared by ConsumeInterrupt(). Same in the 
INTERRUPT_TRANSACTION_TIMEOUT and INTERRUPT_IDLE_SESSION_TIMEOUT 
handling after this.

-- 
Heikki Linnakangas
Neon (https://neon.tech)