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

Hi,

On 2024-08-12 12:55:00 +0300, Heikki Linnakangas wrote:
> While rebasing this today, I spotted another instance of that mistake
> mentioned in the XXX comment above. I called "CountChildren(B_BACKEND)"
> instead of "CountChildren(1 << B_BACKEND)". Some ideas on how to make that
> less error-prone:
>
> 1. Add a separate typedef for the bitmasks, and macros/functions to work
> with it. Something like:
>
> typedef struct {
> uint32 mask;
> } BackendTypeMask;
>
> static const BackendTypeMask BTMASK_ALL = { 0xffffffff };
> static const BackendTypeMask BTMASK_NONE = { 0 };
>
> static inline BackendTypeMask
> BTMASK_ADD(BackendTypeMask mask, BackendType t)
> {
> mask.mask |= 1 << t;
> return mask;
> }
>
> static inline BackendTypeMask
> BTMASK_DEL(BackendTypeMask mask, BackendType t)
> {
> mask.mask &= ~(1 << t);
> return mask;
> }
>
> Now the compiler will complain if you try to pass a BackendType for the
> mask. We could do this just for BackendType, or we could put this in
> src/include/lib/ with a more generic name, like "bitmask_u32".

I don't like the second suggestion - that just ends up creating a similar
problem in the future because flag values for one thing can be passed to
something else.

> +Running the tests
> +=================
> +
> +NOTE: You must have given the --enable-tap-tests argument to configure.
> +
> +Run
> + make check
> +or
> + make installcheck
> +You can use "make installcheck" if you previously did "make install".
> +In that case, the code in the installation tree is tested. With
> +"make check", a temporary installation tree is built from the current
> +sources and then tested.
> +
> +Either way, this test initializes, starts, and stops a test Postgres
> +cluster.
> +
> +See src/test/perl/README for more info about running these tests.

Is it really useful to have such instructions all over the tree?

> From 93b9e9b6e072f63af9009e0d66ab6d0d62ea8c15 Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas <heikki(dot)linnakangas(at)iki(dot)fi>
> Date: Mon, 12 Aug 2024 10:55:11 +0300
> Subject: [PATCH v4 2/8] Add test for dead-end backends
>
> The code path for launching a dead-end backend because we're out of
> slots was not covered by any tests, so add one. (Some tests did hit
> the case of launching a dead-end backend because the server is still
> starting up, though, so the gap in our test coverage wasn't as big as
> it sounds.)
> ---
> src/test/perl/PostgreSQL/Test/Cluster.pm | 39 +++++++++++++++++++
> .../postmaster/t/001_connection_limits.pl | 17 +++++++-
> 2 files changed, 55 insertions(+), 1 deletion(-)

Why does this need to use "raw" connections? Can't you just create a bunch of
connections with BackgroundPsql?

> From 88287a2db95e584018f1c7fa9e992feb7d179ce8 Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas <heikki(dot)linnakangas(at)iki(dot)fi>
> Date: Mon, 12 Aug 2024 10:58:35 +0300
> Subject: [PATCH v4 3/8] Use an shmem_exit callback to remove backend from
> PMChildFlags on exit
>
> This seems nicer than having to duplicate the logic between
> InitProcess() and ProcKill() for which child processes have a
> PMChildFlags slot.
>
> Move the MarkPostmasterChildActive() call earlier in InitProcess(),
> out of the section protected by the spinlock.

> ---
> src/backend/storage/ipc/pmsignal.c | 10 ++++++--
> src/backend/storage/lmgr/proc.c | 38 ++++++++++--------------------
> src/include/storage/pmsignal.h | 1 -
> 3 files changed, 21 insertions(+), 28 deletions(-)
>
> diff --git a/src/backend/storage/ipc/pmsignal.c b/src/backend/storage/ipc/pmsignal.c
> index 27844b46a2..cb99e77476 100644
> --- a/src/backend/storage/ipc/pmsignal.c
> +++ b/src/backend/storage/ipc/pmsignal.c
> @@ -24,6 +24,7 @@
> #include "miscadmin.h"
> #include "postmaster/postmaster.h"
> #include "replication/walsender.h"
> +#include "storage/ipc.h"
> #include "storage/pmsignal.h"
> #include "storage/shmem.h"
> #include "utils/memutils.h"
> @@ -121,6 +122,8 @@ postmaster_death_handler(SIGNAL_ARGS)
>
> #endif /* USE_POSTMASTER_DEATH_SIGNAL */
>
> +static void MarkPostmasterChildInactive(int code, Datum arg);
> +
> /*
> * PMSignalShmemSize
> * Compute space needed for pmsignal.c's shared memory
> @@ -328,6 +331,9 @@ MarkPostmasterChildActive(void)
> slot--;
> Assert(PMSignalState->PMChildFlags[slot] == PM_CHILD_ASSIGNED);
> PMSignalState->PMChildFlags[slot] = PM_CHILD_ACTIVE;
> +
> + /* Arrange to clean up at exit. */
> + on_shmem_exit(MarkPostmasterChildInactive, 0);
> }
>
> /*
> @@ -352,8 +358,8 @@ MarkPostmasterChildWalSender(void)
> * MarkPostmasterChildInactive - mark a postmaster child as done using
> * shared memory. This is called in the child process.
> */
> -void
> -MarkPostmasterChildInactive(void)
> +static void
> +MarkPostmasterChildInactive(int code, Datum arg)
> {
> int slot = MyPMChildSlot;
>
> diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
> index ac66da8638..9536469e89 100644
> --- a/src/backend/storage/lmgr/proc.c
> +++ b/src/backend/storage/lmgr/proc.c
> @@ -308,6 +308,19 @@ InitProcess(void)
> if (MyProc != NULL)
> elog(ERROR, "you already exist");
>
> + /*
> + * Before we start accessing the shared memory in a serious way, mark
> + * ourselves as an active postmaster child; this is so that the postmaster
> + * can detect it if we exit without cleaning up. (XXX autovac launcher
> + * currently doesn't participate in this; it probably should.)
> + *
> + * Slot sync worker also does not participate in it, see comments atop
> + * 'struct bkend' in postmaster.c.
> + */
> + if (IsUnderPostmaster && !AmAutoVacuumLauncherProcess() &&
> + !AmLogicalSlotSyncWorkerProcess())
> + MarkPostmasterChildActive();

I'd not necessarily expect a call to MarkPostmasterChildActive() to register
an shmem exit hook - but I guess it's unlikely to be moved around in a
problematic way. Perhaps something like RegisterPostmasterChild() or such
would be a bit clearer?

> From dc53f89edbeec99f8633def8aa5f47cd98e7a150 Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas <heikki(dot)linnakangas(at)iki(dot)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?

That'd not solve the bitmask confusion issue, but it does seem like a better
direction to me?

> From 9c832ce33667abc5aef128a17fa9c27daaad872a Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas <heikki(dot)linnakangas(at)iki(dot)fi>
> Date: Mon, 12 Aug 2024 10:59:27 +0300
> Subject: [PATCH v4 5/8] Kill dead-end children when there's nothing else left
>
> Previously, the postmaster would never try to kill dead-end child
> processes, even if there were no other processes left. A dead-end
> backend will eventually exit, when authentication_timeout expires, but
> if a dead-end backend is the only thing that's preventing the server
> from shutting down, it seems better to kill it immediately. It's
> particularly important, if there was a bug in the early startup code
> that prevented a dead-end child from timing out and exiting normally.

I do wonder if we shouldn't instead get rid of dead end children. We now have
an event based loop in postmaster, it'd perform vastly better to juts handle
these connections in postmaster. And we'd get rid of these weird backend
types. But I guess this is a worthwhile improvement on its own...

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

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

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

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

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

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

> +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)...

> @@ -1019,7 +980,15 @@ PostmasterMain(int argc, char *argv[])
> /*
> * If enabled, start up syslogger collection subprocess
> */
> - SysLoggerPID = SysLogger_Start();
> + SysLoggerPMChild = AssignPostmasterChildSlot(B_LOGGER);
> + if (!SysLoggerPMChild)
> + elog(ERROR, "no postmaster child slot available for syslogger");
> + SysLoggerPMChild->pid = SysLogger_Start();
> + if (SysLoggerPMChild->pid == 0)
> + {
> + FreePostmasterChildSlot(SysLoggerPMChild);
> + SysLoggerPMChild = NULL;
> + }

Maybe it's a bit obsessive, but this seems long enough to make it worth not
doing inline in the already long PostmasterMain().

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

> @@ -1779,21 +1748,6 @@ canAcceptConnections(int backend_type)
> if (!connsAllowed && backend_type == B_BACKEND)
> return CAC_SHUTDOWN; /* shutdown is pending */
>
> - /*
> - * Don't start too many children.
> - *
> - * 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.
> - *
> - * The limit here must match the sizes of the per-child-process arrays;
> - * see comments for MaxLivePostmasterChildren().
> - */
> - if (CountChildren(BACKEND_TYPE_ALL & ~(1 << B_DEAD_END_BACKEND)) >= MaxLivePostmasterChildren())
> - result = CAC_TOOMANY;
> -
> return result;
> }

It's nice to get rid of this source of O(N^2).

> @@ -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...

> @@ -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?

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

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

> @@ -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...

> @@ -2708,48 +2661,8 @@ HandleChildCrash(int pid, int exitstatus, const char *procname)
> * We could exclude dead_end children here, but at least when
> * sending SIGABRT it seems better to include them.
> */
> - sigquit_child(bp->pid);
> + sigquit_child(bp);
> }
> -
> - if (StartupPID != 0)
> - {
> - sigquit_child(StartupPID);
> - StartupStatus = STARTUP_SIGNALED;
> - }
> -
> - /* Take care of the bgwriter too */
> - if (BgWriterPID != 0)
> - sigquit_child(BgWriterPID);
> -
> - /* Take care of the checkpointer too */
> - if (CheckpointerPID != 0)
> - sigquit_child(CheckpointerPID);
> -
> - /* Take care of the walwriter too */
> - if (WalWriterPID != 0)
> - sigquit_child(WalWriterPID);
> -
> - /* Take care of the walreceiver too */
> - if (WalReceiverPID != 0)
> - sigquit_child(WalReceiverPID);
> -
> - /* Take care of the walsummarizer too */
> - if (WalSummarizerPID != 0)
> - sigquit_child(WalSummarizerPID);
> -
> - /* Take care of the autovacuum launcher too */
> - if (AutoVacPID != 0)
> - sigquit_child(AutoVacPID);
> -
> - /* Take care of the archiver too */
> - if (PgArchPID != 0)
> - sigquit_child(PgArchPID);
> -
> - /* Take care of the slot sync worker too */
> - if (SlotSyncWorkerPID != 0)
> - sigquit_child(SlotSyncWorkerPID);
> -
> - /* We do NOT restart the syslogger */
> }

Yay.

> @@ -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.

> @@ -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.

> @@ -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?

> @@ -311,14 +311,9 @@ InitProcess(void)
> /*
> * Before we start accessing the shared memory in a serious way, mark
> * ourselves as an active postmaster child; this is so that the postmaster
> - * can detect it if we exit without cleaning up. (XXX autovac launcher
> - * currently doesn't participate in this; it probably should.)
> - *
> - * Slot sync worker also does not participate in it, see comments atop
> - * 'struct bkend' in postmaster.c.
> + * can detect it if we exit without cleaning up.
> */
> - if (IsUnderPostmaster && !AmAutoVacuumLauncherProcess() &&
> - !AmLogicalSlotSyncWorkerProcess())
> + if (IsUnderPostmaster)
> MarkPostmasterChildActive();
>
> /* Decide which list should supply our PGPROC. */
> @@ -536,6 +531,9 @@ InitAuxiliaryProcess(void)
> if (MyProc != NULL)
> elog(ERROR, "you already exist");
>
> + if (IsUnderPostmaster)
> + MarkPostmasterChildActive();
> +
> /*
> * We use the ProcStructLock to protect assignment and releasing of
> * AuxiliaryProcs entries.

Probably worth, at some point soon, to have an InitProcessCommon() or such.

Greetings,

Andres Freund