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
