Re: stopgap fix for signal handling during restore_command

Hi,

On 2023-02-25 11:28:25 -0800, Nathan Bossart wrote:
> On Sat, Feb 25, 2023 at 11:07:42AM -0800, Andres Freund wrote:
> > 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

Ick. I guess we've already encountered this, because we've apparently removed
all the (void) write cases. Which I am certain we had at some point. We still
do it for a bunch of other functions though. Ah, yes: aa90e148ca7,
27314d32a88, 6c72a28e5ce etc.

I think I opined on this before, but we really ought to have a function to do
some minimal signal safe output. Implemented centrally, instead of being open
coded in a bunch of places.

> >> 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?

Because we so far have little to no protection against some state corruption
leading to releasing PGPROC that's not ours. I didn't actually mean that we
shouldn't check that MyProcPid == (int) getpid(), just that the much more
interesting case to check is that MyProc->pid matches, because that protect
against multiple releases, releasing the wrong slot, etc.

Greetings,

Andres Freund