Re: stopgap fix for signal handling during restore_command - Mailing list pgsql-hackers

From Andres Freund
Subject Re: stopgap fix for signal handling during restore_command
Date
Msg-id 20230225195253.hzp6pyllhn4kw4g6@awork3.anarazel.de
Whole thread Raw
In response to Re: stopgap fix for signal handling during restore_command  (Nathan Bossart <nathandbossart@gmail.com>)
Responses Re: stopgap fix for signal handling during restore_command
List pgsql-hackers
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
attributewarn_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



pgsql-hackers by date:

Previous
From: Nathan Bossart
Date:
Subject: Re: stopgap fix for signal handling during restore_command
Next
From: Melanie Plageman
Date:
Subject: Add shared buffer hits to pg_stat_io