Re: Refactoring postmaster's code to cleanup after child exit - Mailing list pgsql-hackers

On 04/09/2024 17:35, Andres Freund wrote:
> On 2024-08-12 12:55:00 +0300, Heikki Linnakangas wrote:
>>  From dc53f89edbeec99f8633def8aa5f47cd98e7a150 Mon Sep 17 00:00:00 2001
>> From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
>> Date: Mon, 12 Aug 2024 10:59:04 +0300
>> Subject: [PATCH v4 4/8] Introduce a separate BackendType for dead-end children
>>
>> And replace postmaster.c's own "backend type" codes with BackendType
> 
> Hm - it seems a bit odd to open-code this when we actually have a "table
> driven configuration" available?  Why isn't the type a field in
> child_process_kind?

Sorry, I didn't understand this. What exactly would you add to 
child_process_kind? Where would you use it?

>> +/*
>> + * MaxLivePostmasterChildren
>> + *
>> + * This reports the number postmaster child processes that can be active.  It
>> + * includes all children except for dead_end children.  This allows the array
>> + * in shared memory (PMChildFlags) to have a fixed maximum size.
>> + */
>> +int
>> +MaxLivePostmasterChildren(void)
>> +{
>> +    int            n = 0;
>> +
>> +    /* We know exactly how mamy worker and aux processes can be active */
>> +    n += autovacuum_max_workers;
>> +    n += max_worker_processes;
>> +    n += NUM_AUXILIARY_PROCS;
>> +
>> +    /*
>> +     * We allow more connections here than we can have backends because some
>> +     * might still be authenticating; they might fail auth, or some existing
>> +     * backend might exit before the auth cycle is completed.  The exact
>> +     * MaxBackends limit is enforced when a new backend tries to join the
>> +     * shared-inval backend array.
>> +     */
>> +    n += 2 * (MaxConnections + max_wal_senders);
>> +
>> +    return n;
>> +}
> 
> I wonder if we could instead maintain at least some of this in
> child_process_kinds? Manually listing different types of processes in
> different places doesn't seem particularly sustainable.

Hmm, you mean adding "max this kind of children" field to 
child_process_kinds? Perhaps.

> 
>> +/*
>> + * Initialize at postmaster startup
>> + */
>> +void
>> +InitPostmasterChildSlots(void)
>> +{
>> +    int            num_pmchild_slots;
>> +    int            slotno;
>> +    PMChild    *slots;
>> +
>> +    dlist_init(&freeBackendList);
>> +    dlist_init(&freeAutoVacWorkerList);
>> +    dlist_init(&freeBgWorkerList);
>> +    dlist_init(&freeAuxList);
>> +    dlist_init(&ActiveChildList);
>> +
>> +    num_pmchild_slots = MaxLivePostmasterChildren();
>> +
>> +    slots = palloc(num_pmchild_slots * sizeof(PMChild));
>> +
>> +    slotno = 0;
>> +    for (int i = 0; i < 2 * (MaxConnections + max_wal_senders); i++)
>> +    {
>> +        init_slot(&slots[slotno], slotno, &freeBackendList);
>> +        slotno++;
>> +    }
>> +    for (int i = 0; i < autovacuum_max_workers; i++)
>> +    {
>> +        init_slot(&slots[slotno], slotno, &freeAutoVacWorkerList);
>> +        slotno++;
>> +    }
>> +    for (int i = 0; i < max_worker_processes; i++)
>> +    {
>> +        init_slot(&slots[slotno], slotno, &freeBgWorkerList);
>> +        slotno++;
>> +    }
>> +    for (int i = 0; i < NUM_AUXILIARY_PROCS; i++)
>> +    {
>> +        init_slot(&slots[slotno], slotno, &freeAuxList);
>> +        slotno++;
>> +    }
>> +    Assert(slotno == num_pmchild_slots);
>> +}
> 
> Along the same vein - could we generalize this into one array of different
> slot types and then loop over that to initialize / acquire the slots?

Makes sense.

>> +/* 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.

> 
>> +            /*
>> +             * Auxiliary processes.  There can be only one of each of these
>> +             * running at a time.
>> +             */
>> +        case B_AUTOVAC_LAUNCHER:
>> +        case B_ARCHIVER:
>> +        case B_BG_WRITER:
>> +        case B_CHECKPOINTER:
>> +        case B_STARTUP:
>> +        case B_WAL_RECEIVER:
>> +        case B_WAL_SUMMARIZER:
>> +        case B_WAL_WRITER:
>> +            return &freeAuxList;
>> +
>> +            /*
>> +             * Logger is not connected to shared memory, and does not have a
>> +             * PGPROC entry, but we still allocate a child slot for it.
>> +             */
> 
> 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.Currently, the # of slots reserved for aux 
processes is sized by NUM_AUXILIARY_PROCS, which is one smaller than the 
number of different aux proces kinds:

> /*
>  * We set aside some extra PGPROC structures for auxiliary processes,
>  * ie things that aren't full-fledged backends but need shmem access.
>  *
>  * Background writer, checkpointer, WAL writer, WAL summarizer, and archiver
>  * run during normal operation.  Startup process and WAL receiver also consume
>  * 2 slots, but WAL writer is launched only after startup has exited, so we
>  * only need 6 slots.
>  */
> #define NUM_AUXILIARY_PROCS        6

For PMChildSlot numbers, we could certainly just allocate one more slot.

It would probably make sense for PGPROCs too, even though PGPROC is a 
much larger struct.

>> +PMChild *
>> +AssignPostmasterChildSlot(BackendType btype)
>> +{
>> +    dlist_head *freelist;
>> +    PMChild    *pmchild;
>> +
>> +    freelist = GetFreeList(btype);
>> +
>> +    if (dlist_is_empty(freelist))
>> +        return NULL;
>> +
>> +    pmchild = dlist_container(PMChild, elem, dlist_pop_head_node(freelist));
>> +    pmchild->pid = 0;
>> +    pmchild->bkend_type = btype;
>> +    pmchild->rw = NULL;
>> +    pmchild->bgworker_notify = true;
>> +
>> +    /*
>> +     * pmchild->child_slot for each entry was initialized when the array of
>> +     * slots was allocated.
>> +     */
>> +
>> +    dlist_push_head(&ActiveChildList, &pmchild->elem);
>> +
>> +    ReservePostmasterChildSlot(pmchild->child_slot);
>> +
>> +    /* FIXME: find a more elegant way to pass this */
>> +    MyPMChildSlot = pmchild->child_slot;
> 
> What if we assigned one offset for each process and assigned its ID here and
> also used that for its ProcNumber - that way we wouldn't need to manage
> freelists in two places.

It's currently possible to have up to 2 * max_connections backends in 
the authentication phase. We would have to change that behaviour, or 
make the PGPROC array 2x larger.

It might well be worth it, I don't know how sensible the current 
behaviour is. But I'd like to punt that to later patch, to keep the 
scope of this patch set reasonable. It's pretty straightforward to do 
later on top of this if we want to.

>> +PMChild *
>> +FindPostmasterChildByPid(int pid)
>> +{
>> +    dlist_iter    iter;
>> +
>> +    dlist_foreach(iter, &ActiveChildList)
>> +    {
>> +        PMChild    *bp = dlist_container(PMChild, elem, iter.cur);
>> +
>> +        if (bp->pid == pid)
>> +            return bp;
>> +    }
>> +    return NULL;
>> +}
> 
> It's not new, but it's quite sad that postmaster's process exit handling is
> effectively O(Backends^2)...

It would be straightforward to turn ActiveChildList into a hash table. 
But I'd like to leave that to a followup patch too.

>>       /*
>>        * We're ready to rock and roll...
>>        */
>> -    StartupPID = StartChildProcess(B_STARTUP);
>> -    Assert(StartupPID != 0);
>> +    StartupPMChild = StartChildProcess(B_STARTUP);
>> +    Assert(StartupPMChild != NULL);
> 
> This (not new) assertion is ... odd.

Yeah, it's an assertion because StartChildProcess has this:

>         /*
>          * fork failure is fatal during startup, but there's no need to choke
>          * immediately if starting other child types fails.
>          */
>         if (type == B_STARTUP)
>             ExitPostmaster(1);


>> @@ -1961,26 +1915,6 @@ process_pm_reload_request(void)
>>                   (errmsg("received SIGHUP, reloading configuration files")));
>>           ProcessConfigFile(PGC_SIGHUP);
>>           SignalSomeChildren(SIGHUP, BACKEND_TYPE_ALL & ~(1 << B_DEAD_END_BACKEND));
>> -        if (StartupPID != 0)
>> -            signal_child(StartupPID, SIGHUP);
>> -        if (BgWriterPID != 0)
>> -            signal_child(BgWriterPID, SIGHUP);
>> -        if (CheckpointerPID != 0)
>> -            signal_child(CheckpointerPID, SIGHUP);
>> -        if (WalWriterPID != 0)
>> -            signal_child(WalWriterPID, SIGHUP);
>> -        if (WalReceiverPID != 0)
>> -            signal_child(WalReceiverPID, SIGHUP);
>> -        if (WalSummarizerPID != 0)
>> -            signal_child(WalSummarizerPID, SIGHUP);
>> -        if (AutoVacPID != 0)
>> -            signal_child(AutoVacPID, SIGHUP);
>> -        if (PgArchPID != 0)
>> -            signal_child(PgArchPID, SIGHUP);
>> -        if (SysLoggerPID != 0)
>> -            signal_child(SysLoggerPID, SIGHUP);
>> -        if (SlotSyncWorkerPID != 0)
>> -            signal_child(SlotSyncWorkerPID, SIGHUP);
>>   
>>           /* Reload authentication config files too */
>>           if (!load_hba())
> 
> For a moment I wondered why this change was part of this commit - but I guess
> we didn't have any of these in an array/list before this change...

Correct.

>> @@ -2469,11 +2410,15 @@ process_pm_child_exit(void)
>>           }
>>   
>>           /* Was it the system logger?  If so, try to start a new one */
>> -        if (pid == SysLoggerPID)
>> +        if (SysLoggerPMChild && pid == SysLoggerPMChild->pid)
>>           {
>> -            SysLoggerPID = 0;
>>               /* for safety's sake, launch new logger *first* */
>> -            SysLoggerPID = SysLogger_Start();
>> +            SysLoggerPMChild->pid = SysLogger_Start();
>> +            if (SysLoggerPMChild->pid == 0)
>> +            {
>> +                FreePostmasterChildSlot(SysLoggerPMChild);
>> +                SysLoggerPMChild = NULL;
>> +            }
>>               if (!EXIT_STATUS_0(exitstatus))
>>                   LogChildExit(LOG, _("system logger process"),
> 
> Seems a bit weird to have one place with a different memory lifetime handling
> than other places. Why don't we just do this the same way as in other places
> but continue to defer the logging until after we tried to start the new
> logger?

Hmm, you mean let LaunchMissingBackgroundProcesses() handle the restart?

I'm a little scared of changing the existing logic. We don't have a 
mechanism for deferring logging, so we would have to invent that, or the 
logs would just accumulate in the pipe until syslogger starts up. 
There's some code between here and LaunchMissingBackgroundProcesses(), 
so might postmaster get blocked between writing to the syslogger pipe, 
before having restarted it?

If forking the syslogger process fails, that can happen anyway, though.

> Might be worth having a test ensuring that loggers restart OK.

Yeah..

>>       /* 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?

Constructing the string for background workers is a little more complicated:

     snprintf(namebuf, MAXPGPATH, _("background worker \"%s\""),
              bp->rw->rw_worker.bgw_type);

We could still do that for background workers and use the transalated 
variant of GetBackendTypeDesc() for everything else though.

>> @@ -2697,9 +2643,16 @@ HandleChildCrash(int pid, int exitstatus, const char *procname)
>>       {
>>           dlist_iter    iter;
>>   
>> -        dlist_foreach(iter, &BackendList)
>> +        dlist_foreach(iter, &ActiveChildList)
>>           {
>> -            Backend    *bp = dlist_container(Backend, elem, iter.cur);
>> +            PMChild    *bp = dlist_container(PMChild, elem, iter.cur);
>> +
>> +            /* We do NOT restart the syslogger */
>> +            if (bp == SysLoggerPMChild)
>> +                continue;
> 
> That comment seems a bit misleading - we do restart syslogger, we just don't
> do it here, no?  I realize it's an old comment, but it still seems like it's
> worth fixing given that you touch all the code here...

No, we really do not restart the syslogger. This code runs when 
*another* process has crashed unexpectedly. We kill all other processes, 
reinitialize shared memory and restart, but the old syslogger keeps 
running through all that.

I'll add a note on that to InitPostmasterChildSlots(), as it's a bit 
surprising.

>> @@ -2871,29 +2786,27 @@ PostmasterStateMachine(void)
>> <snip>
>> -        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;
>> <snip>
> 
> It's likely the right thing to not do as one patch, but IMO this really wants
> to be a state table. Perhaps as part of child_process_kinds, perhaps separate
> from that.

Yeah. I've tried to refactor this into a table before, but didn't come 
up with anything that I was happy with. I also feel there must be a 
better way to organize this, but not sure what exactly. I hope that will 
become more apparent after these other changes.

>> @@ -3130,8 +3047,21 @@ static void
>>   LaunchMissingBackgroundProcesses(void)
>>   {
>>       /* Syslogger is active in all states */
>> -    if (SysLoggerPID == 0 && Logging_collector)
>> -        SysLoggerPID = SysLogger_Start();
>> +    if (SysLoggerPMChild == NULL && Logging_collector)
>> +    {
>> +        SysLoggerPMChild = AssignPostmasterChildSlot(B_LOGGER);
>> +        if (!SysLoggerPMChild)
>> +            elog(LOG, "no postmaster child slot available for syslogger");
> 
> How could this elog() be reached? Seems something seriously would have gone
> wrong to get here - in which case a LOG that might not even be visible (due to
> logger not working) doesn't seem like the right response.

I'll turn it into an assertion or PANIC.

>> @@ -3334,29 +3270,12 @@ SignalSomeChildren(int signal, uint32 targetMask)
>>   static void
>>   TerminateChildren(int signal)
>>   {
> 
> The comment for TerminateChildren() says "except syslogger and dead_end
> backends." - aren't you including the latter here?

The comment is adjusted in 
v4-0004-Introduce-a-separate-BackendType-for-dead-end-chi.patch. Before 
that, SignalChildren() does ignore dead-end children.

Thanks for the review!

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