Minor postmaster state machine bugs

In pursuit of the problem with standby servers sometimes not responding
to fast shutdowns [1], I spent awhile staring at the postmaster's
state-machine logic. I have not found a cause for that problem,
but I have identified some other things that seem like bugs:

1. sigusr1_handler ignores PMSIGNAL_ADVANCE_STATE_MACHINE unless the
current state is PM_WAIT_BACKUP or PM_WAIT_BACKENDS. This restriction
seems useless and shortsighted: PostmasterStateMachine should behave
sanely regardless of our state, and sigusr1_handler really has no
business assuming anything about why a child is asking for a state
machine reconsideration. But it's not just not future-proof, it's a
live bug even for the one existing use-case, which is that a new
walsender sends this signal after it's re-marked itself as being a
walsender rather than a normal backend. Consider this sequence of
events:
* System is running as a hot standby and allowing cascaded replication.
There are no live backends.
* New replication connection request is received and forked off.
(At this point the postmaster thinks this child is a normal session
backend.)
* SIGTERM (Smart Shutdown) is received. Postmaster will transition
to PM_WAIT_READONLY. I don't think it would have autovac or bgworker
or bgwriter or walwriter children, but if so, assume they all exit
before the next step. Postmaster will continue to sleep, waiting for
its one "normal" child backend to finish.
* Replication connection request completes, so child re-marks itself
as a walsender and sends PMSIGNAL_ADVANCE_STATE_MACHINE.
* Postmaster ignores signal because it's in the "wrong" state, so it
doesn't realize it now has no normal backend children.
* Postmaster waits forever, or at least till DBA loses patience and
sends a stronger signal.

This scenario doesn't explain the buildfarm failures since those don't
involve smart shutdowns (and I think they don't involve cascaded
replication either). Still, it's clearly a bug, which I think
we should fix by removing the pointless restriction on whether
PostmasterStateMachine can be called.

Also, I'm inclined to think that that should be the *last* step in
sigusr1_handler, not randomly somewhere in the middle. As coded,
it's basically assuming that no later action in sigusr1_handler
could affect anything that PostmasterStateMachine cares about, which
even if it's true today is another highly not-future-proof assumption.

2. MaybeStartWalReceiver will clear the WalReceiverRequested flag
even if it fails to launch a child process for some reason. This
is just dumb; it should leave the flag set so that we'll try again
next time through the postmaster's idle loop.

3. PostmasterStateMachine's handling of PM_SHUTDOWN_2 is:

if (pmState == PM_SHUTDOWN_2)
{
/*
* PM_SHUTDOWN_2 state ends when there's no other children than
* dead_end children left. There shouldn't be any regular backends
* left by now anyway; what we're really waiting for is walsenders and
* archiver.
*
* Walreceiver should normally be dead by now, but not when a fast
* shutdown is performed during recovery.
*/
if (PgArchPID == 0 && CountChildren(BACKEND_TYPE_ALL) == 0 &&
WalReceiverPID == 0)
{
pmState = PM_WAIT_DEAD_END;
}
}

The comment about walreceivers is confusing, and it's also wrong. Maybe
it was valid when written, but today it's easy to trace the logic and see
that we can only get to PM_SHUTDOWN_2 state from PM_SHUTDOWN state, and
we can only get to PM_SHUTDOWN state when there is no live walreceiver
(cf processing of PM_WAIT_BACKENDS state), and we won't attempt to launch
a new walreceiver while in PM_SHUTDOWN or PM_SHUTDOWN_2 state, so it's
impossible for there to be any walreceiver here. I think we should just
remove that comment and the WalReceiverPID == 0 test.

Comments? I think at least the first two points need to be back-patched.

regards, tom lane

[1] https://www.postgresql.org/message-id/20190416070119.GK2673@paquier.xyz