Re: Reorder shutdown sequence, to flush pgstats later

Hi,

On Fri, Jan 24, 2025 at 03:06:17PM -0500, Andres Freund wrote:
> On 2025-01-15 08:38:33 +0000, Bertrand Drouvot wrote:
> > Fair enough. I'll look at the remaining pieces once this stuff goes in.
> 
> Cool. I did try writing the change, but it does indeed seem better as a
> separate patch. Besides the error message, it also seems we ought to invent a
> different ERRCODE, as neither ERRCODE_ADMIN_SHUTDOWN nor
> ERRCODE_CRASH_SHUTDOWN seem appropriate.

Thanks for the feedback, will look at it.

> Vaguely related and not at all crucial:
> 
> quickdie(), in the PMQUIT_FOR_CRASH, does
>                      errhint("In a moment you should be able to reconnect to the"
>                              " database and repeat your command.")));
> 
> but in case of an immediate *restart* (via pg_ctl restart -m immediate) we'll
> not have such hint. postmaster.c doesn't know it's an immediate restart, so
> that's not surprising, but it still seems a bit weird. One would hope that
> immediate restarts are more common than crashes...

Yeah, it does not make the distinction between an "immediate restart" and an
"immediate stop". I agree that such hint "should" be added in case of "immediate
restart" (and keep "immediate stop" as it is): will give it a look.

> > 
> > > > But that would also need to call TerminateChildren() (a second time) again after
> > > > ConfigurePostmasterWaitSet() which is not ideal though (unless we move the
> > > > TerminateChildren() calls in the switch or add a check on PM_WAIT_DEAD_END in
> > > > the first call).
> > > 
> > > Why would it need to?
> > 
> > Because I thought that TerminateChildren() needs to be called after
> > ConfigurePostmasterWaitSet(false). If not, could new connections be accepted in
> > the window between TerminateChildren() and ConfigurePostmasterWaitSet(false)?
> 
> I don't think that can happen with the attached patch - the
> ConfigurePostmasterWaitSet() is called before HandleFatalError() returns, so
> no new connections can be accepted, as that happens only from
> ServerLoop()->AcceptConnection().

Thanks for the explanation, that was the missing part in my reasoning. So yeah,
all good as no new connections are accepted.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com