From: Nathan Bossart <nathandbossart@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andres Freund <andres@anarazel.de>
Cc: Thomas Munro <thomas.munro@gmail.com>
Cc: Fujii Masao <fujii@postgresql.org>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Weird failure with latches in curculio on v15
Date: Thu, 2 Feb 2023 14:01:13 -0800
Message-ID: <20230202220113.GA3945808@nathanxps13> (raw)
In-Reply-To: <CA+Tgmob+KZQn_EfVOp9umWc6iJmuzn5oH9hF_9+Cdu=wPgiEZg@mail.gmail.com>
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>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, robertmhaas@gmail.com, michael@paquier.xyz, tgl@sss.pgh.pa.us, andres@anarazel.de, thomas.munro@gmail.com, fujii@postgresql.org, pgsql-hackers@lists.postgresql.org
Subject: Re: Weird failure with latches in curculio on v15
In-Reply-To: <20230202220113.GA3945808@nathanxps13>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox