From: Nathan Bossart <nathandbossart@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: Thomas Munro <thomas.munro@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Fujii Masao <fujii@postgresql.org>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: stopgap fix for signal handling during restore_command
Date: Sat, 25 Feb 2023 11:28:25 -0800
Message-ID: <20230225192825.GA868928@nathanxps13> (raw)
In-Reply-To: <20230225190742.lfopue3nkrvxxoie@awork3.anarazel.de>
References: <20230223231503.GA743455@nathanxps13>
<CA+hUKG+d+u2BZu=N4q15EbR7s52KugSQeQ8eUXE8iuEBe108ng@mail.gmail.com>
<20230224043323.GA821522@nathanxps13>
<20230225190742.lfopue3nkrvxxoie@awork3.anarazel.de>
On Sat, Feb 25, 2023 at 11:07:42AM -0800, Andres Freund wrote:
> On 2023-02-23 20:33:23 -0800, Nathan Bossart wrote:>
>> if (in_restore_command)
>> - proc_exit(1);
>> + {
>> + /*
>> + * If we are in a child process (e.g., forked by system() in
>> + * RestoreArchivedFile()), we don't want to call any exit callbacks.
>> + * The parent will take care of that.
>> + */
>> + if (MyProcPid == (int) getpid())
>> + proc_exit(1);
>> + else
>> + {
>> + const char msg[] = "StartupProcShutdownHandler() called in child process\n";
>> + int rc pg_attribute_unused();
>> +
>> + rc = write(STDERR_FILENO, msg, sizeof(msg));
>> + _exit(1);
>> + }
>> + }
>
> Why do we need that rc variable? Don't we normally get away with (void)
> write(...)?
My compiler complains about that. :/
../postgresql/src/backend/postmaster/startup.c: In function ‘StartupProcShutdownHandler’:
../postgresql/src/backend/postmaster/startup.c:139:11: error: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Werror=unused-result]
139 | (void) write(STDERR_FILENO, msg, sizeof(msg));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
>> diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
>> index 22b4278610..e3da0622d7 100644
>> --- a/src/backend/storage/lmgr/proc.c
>> +++ b/src/backend/storage/lmgr/proc.c
>> @@ -805,6 +805,7 @@ ProcKill(int code, Datum arg)
>> dlist_head *procgloballist;
>>
>> Assert(MyProc != NULL);
>> + Assert(MyProcPid == (int) getpid()); /* not safe if forked by system(), etc. */
>>
>> /* Make sure we're out of the sync rep lists */
>> SyncRepCleanupAtProcExit();
>> @@ -925,6 +926,7 @@ AuxiliaryProcKill(int code, Datum arg)
>> PGPROC *proc;
>>
>> Assert(proctype >= 0 && proctype < NUM_AUXILIARY_PROCS);
>> + Assert(MyProcPid == (int) getpid()); /* not safe if forked by system(), etc. */
>>
>> auxproc = &AuxiliaryProcs[proctype];
>>
>> --
>> 2.25.1
>
> I think the much more interesting assertion here would be to check that
> MyProc->pid equals the current pid.
I don't mind changing this, but why is this a more interesting assertion?
--
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, andres@anarazel.de, thomas.munro@gmail.com, michael@paquier.xyz, robertmhaas@gmail.com, tgl@sss.pgh.pa.us, fujii@postgresql.org, pgsql-hackers@lists.postgresql.org
Subject: Re: stopgap fix for signal handling during restore_command
In-Reply-To: <20230225192825.GA868928@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