Re: Refactoring postmaster's code to cleanup after child exit

I pushed the first three patches, with the new test and one of the small 
refactoring patches. Thanks for all the comments so far! Here is a new 
version of the remaining patches.

Lots of little cleanups and changes here and there since the last 
versions, but the notable bigger changes are:

- There is now a BackendTypeMask datatype, so that if you try to mix up 
bitmasks and plain BackendType values, the compiler will complain.

- pmchild.c has been rewritten per feedback, so that the "pools" of 
PMChild structs are more explicit. The size of each pool is only stated 
once, whereas before the same logic was duplicated in 
MaxLivePostmasterChildren() which calculates the number of slots and in 
InitPostmasterChildSlots() which allocates them.

- In PostmasterStateMachine(), I combined the code to handle 
PM_STOP_BACKENDS and PM_WAIT_BACKENDS. They are essentially the same 
state, except that PM_STOP_BACKENDS first sends the signal to all the 
child processes that it will then wait for. They both needed to build 
the same bitmask of processes to signal or wait for; this eliminates the 
duplication.

Responses to some specific comments below:

On 10/09/2024 20:53, Andres Freund wrote:
> On 2024-08-12 12:55:00 +0300, Heikki Linnakangas wrote:
>> @@ -2864,6 +2777,8 @@ PostmasterStateMachine(void)
>>   	 */
>>   	if (pmState == PM_STOP_BACKENDS)
>>   	{
>> +		uint32		targetMask;
>> +
>>   		/*
>>   		 * Forget any pending requests for background workers, since we're no
>>   		 * longer willing to launch any new workers.  (If additional requests
>> @@ -2871,29 +2786,27 @@ PostmasterStateMachine(void)
>>   		 */
>>   		ForgetUnstartedBackgroundWorkers();
>>   
>> -		/* Signal all backend children except walsenders and dead-end backends */
>> -		SignalSomeChildren(SIGTERM,
>> -						   BACKEND_TYPE_ALL & ~(1 << B_WAL_SENDER | 1 << B_DEAD_END_BACKEND));
>> +		/* Signal all backend children except walsenders */
>> +		/* dead-end children are not signalled yet */
>> +		targetMask = (1 << B_BACKEND);
>> +		targetMask |= (1 << B_BG_WORKER);
>> +
>>   		/* and the autovac launcher too */
>> -		if (AutoVacPID != 0)
>> -			signal_child(AutoVacPID, SIGTERM);
>> +		targetMask |= (1 << B_AUTOVAC_LAUNCHER);
>>   		/* and the bgwriter too */
>> -		if (BgWriterPID != 0)
>> -			signal_child(BgWriterPID, SIGTERM);
>> +		targetMask |= (1 << B_BG_WRITER);
>>   		/* and the walwriter too */
>> -		if (WalWriterPID != 0)
>> -			signal_child(WalWriterPID, SIGTERM);
>> +		targetMask |= (1 << B_WAL_WRITER);
>>   		/* If we're in recovery, also stop startup and walreceiver procs */
>> -		if (StartupPID != 0)
>> -			signal_child(StartupPID, SIGTERM);
>> -		if (WalReceiverPID != 0)
>> -			signal_child(WalReceiverPID, SIGTERM);
>> -		if (WalSummarizerPID != 0)
>> -			signal_child(WalSummarizerPID, SIGTERM);
>> -		if (SlotSyncWorkerPID != 0)
>> -			signal_child(SlotSyncWorkerPID, SIGTERM);
>> +		targetMask |= (1 << B_STARTUP);
>> +		targetMask |= (1 << B_WAL_RECEIVER);
>> +
>> +		targetMask |= (1 << B_WAL_SUMMARIZER);
>> +		targetMask |= (1 << B_SLOTSYNC_WORKER);
>>   		/* checkpointer, archiver, stats, and syslogger may continue for now */
>>   
>> +		SignalSomeChildren(SIGTERM, targetMask);
>> +
>>   		/* Now transition to PM_WAIT_BACKENDS state to wait for them to die */
>>   		pmState = PM_WAIT_BACKENDS;
>>   	}
> 
> I think this might now omit shutting down at least autovac workers, which
> afaict previously were included.

Fixed. And this code now also explicitly lists backend types that are 
*not* signaled, and there is an assertion that all backend types are 
accounted for. Thanks to that, if someone adds a new backend type, they 
will be forced to decide if the new backend type should be signaled here 
or not. That's not quite table-driven like you suggested, but it's 
closer to that.

>> > > +/* Return the appropriate free-list for the given backend type */
>> > > +static dlist_head *
>> > > +GetFreeList(BackendType btype)
>> > > +{
>> > > +	switch (btype)
>> > > +	{
>> > > +		case B_BACKEND:
>> > > +		case B_BG_WORKER:
>> > > +		case B_WAL_SENDER:
>> > > +		case B_SLOTSYNC_WORKER:
>> > > +			return &freeBackendList;
>> >
>> > Maybe a daft question - but why are all of these in the same list? Sure,
>> > they're essentially all full backends, but they're covered by different GUCs?
>>
>> No reason. No particular reason they should *not* share the same list either
>> though.
> 
> 
> Aren't they controlled by distinct connection limits? Isn't there a danger
> that we could use up entries and fail connections due to that, despite not
> actually being above the limit?

Yes, this was in fact just wrong. Slotsync worker is a special process 
and should not be allocated from the same pool as backends, and 
previously it was not. And indeed bgworkers have a separate connection 
limit, and should have a separate pool. Fixed.

>> > Tangential: Why do we need a freelist for these and why do we choose a random
>> > pgproc for these instead of assigning one statically?
>> >
>> > Background: I'd like to not provide AIO workers with "bounce buffers" (for IO
>> > of buffers that can't be done in-place, like writes when checksums are
>> > enabled). The varying proc numbers make that harder than it'd have to be...
>>
>> Yeah, we can make these fixed.
> 
> Cool.

All the aux processes now have their own "free list" or pool of a single 
entry now, so after postmaster startup, their child_slot never changes. 
They're still not constants across server startups though, because the 
numbering depends on max_connections etc. If it matters, we could 
allocate the aux process slot numbers first, so that they would be truly 
static, but I didn't do that.

I did not change how ProcNumbers are allocated. They are still separate 
from PMChildSlots.

>> Includes a test for that case where a dead-end backend previously kept
>> the server from shutting down.
> 
> The test hardcodes timeouts, I think we've largely come to regret that when we
> did. Should probably just be a multiplier based on
> PostgreSQL::Test::Utils::timeout_default?

Hmm, what the test really needs is that the authentication_timeout is >> 
the "pg_ctl stop" timeout. The idea is that if a dead-end backend isn't 
killed, but needs to wait for authentication_timeout to expire, the test 
should fail. The default pg_ctl stop timeout is actually only 60 s, 
while PostgreSQL::Test::Utils::timeout_default is 180 s.

I changed authentication_timeout in the test to 
PostgreSQL::Test::Utils::timeout_default, but also added an explicit 
timeout to "$node->stop", and set that to authentication_timeout / 2. 
That ensures that the stop timeout is smaller than 
authentication_timeout, regardless of 
PostgreSQL::Test::Utils::timeout_default or the default pg_ctl timeout.

>>  	/* Construct a process name for log message */
>> +
>> +	/*
>> +	 * FIXME: use GetBackendTypeDesc here? How does the localization of that
>> +	 * work?
>> +	 */
>>  	if (bp->bkend_type == B_DEAD_END_BACKEND)
>>  	{
>>  		procname = _("dead end backend");
> 
> Might be worth having a version of GetBackendTypeDesc() that returns a
> translated string?

I made the translation of GetBackendTypeDesc() work the same as for 
error_severity(int elevel).

-- 
Heikki Linnakangas
Neon (https://neon.tech)