Date: Thu, 2 Feb 2023 14:01:13 -0800
From: Nathan Bossart <nathandbossart@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>, Tom Lane <tgl@sss.pgh.pa.us>,
	Andres Freund <andres@anarazel.de>,
	Thomas Munro <thomas.munro@gmail.com>,
	Fujii Masao <fujii@postgresql.org>,
	Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Weird failure with latches in curculio on v15
Message-ID: <20230202220113.GA3945808@nathanxps13>
References: <20230201105514.rsjl4bnhb65giyvo@alap3.anarazel.de>
 <1369666.1675264346@sss.pgh.pa.us>
 <20230201165801.33ydbxvjdbomjqa7@alap3.anarazel.de>
 <20230201175806.GA3199959@nathanxps13>
 <20230201223555.GA3721373@nathanxps13>
 <Y9sam108o4mxZFiS@paquier.xyz>
 <1449633.1675305284@sss.pgh.pa.us>
 <Y9s678gkiX0pEb5C@paquier.xyz>
 <20230202200957.GA3944544@nathanxps13>
 <CA+Tgmob+KZQn_EfVOp9umWc6iJmuzn5oH9hF_9+Cdu=wPgiEZg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CA+Tgmob+KZQn_EfVOp9umWc6iJmuzn5oH9hF_9+Cdu=wPgiEZg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20230202220113.GA3945808%40nathanxps13>
Precedence: bulk

On Thu, Feb 02, 2023 at 04:14:54PM -0500, Robert Haas wrote:
> +       /*
> +        * When exitOnSigterm is set and we are in the startup process, use the
> +        * special wrapper for system() that enables exiting immediately upon
> +        * receiving SIGTERM.  This ensures we can break out of system() if
> +        * required.
> +        */
> 
> This comment, for me, raises more questions than it answers. Why do we
> only do this in the startup process?

Currently, this functionality only exists in the startup process because it
is only used for restore_command.  More below...

> Also, and this part is not the fault of this patch but a defect of the
> pre-existing comments, under what circumstances do we not want to exit
> when we get a SIGTERM? It's standard behavior for PostgreSQL backends
> to exit when they receive SIGTERM, so the question isn't why we
> sometimes exit immediately but why we ever don't. The existing code
> calls ExecuteRecoveryCommand with exitOnSigterm true in some cases and
> false in other cases, and AFAICS there are zero words of comments
> explaining the reasoning.

I've been digging into the history here.  This e-mail seems to have the
most context [0].  IIUC this was intended to prevent "fast" shutdowns from
escalating to "immediate" shutdowns because the restore command died
unexpectedly.  This doesn't apply to archive_cleanup_command because we
don't FATAL if it dies unexpectedly.  It seems like this idea should apply
to recovery_end_command, too, but AFAICT it doesn't use the same approach.
My guess is that this hasn't come up because it's less likely that both 1)
recovery_end_command is used and 2) someone initiates shutdown while it is
running.

BTW the relevant commits are cdd46c7 (added SIGTERM handling for
restore_command), 9e403c2 (added recovery_end_command), and c21ac0b (added
what is today called archive_cleanup_command).

> +       if (exitOnSigterm && MyBackendType == B_STARTUP)
> +               rc = RunInterruptibleShellCommand(command);
> +       else
> +               rc = system(command);
> 
> And this looks like pure magic. I'm all in favor of not relying on
> system(), but using it under some opaque set of conditions and
> otherwise doing something else is not the way. At the very least this
> needs to be explained a whole lot better.

If we applied this exit-on-SIGTERM behavior to recovery_end_command, I
think we could combine failOnSignal and exitOnSigterm into one flag, and
then it might be a little easier to explain what is going on.  In any case,
I agree that this deserves a lengthy explanation, which I'll continue to
work on.

[0] https://postgr.es/m/499047FE.9090407%40enterprisedb.com

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com