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

From: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
To: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
Cc: "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Refactoring postmaster's code to cleanup after child exit
Date: 2024-08-12 09:55:00
Message-ID: a102f15f-eac4-4ff2-af02-f9ff209ec66f@iki.fi
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 10/08/2024 00:13, Heikki Linnakangas wrote:
> On 08/08/2024 13:47, Thomas Munro wrote:
>>> * v3-0004-Consolidate-postmaster-code-to-launch-background-.patch
>>
>>      Much of the code in process_pm_child_exit() to launch replacement
>>      processes when one exits or when progressing to next postmaster
>> state
>>      was unnecessary, because the ServerLoop will launch any missing
>>      background processes anyway. Remove the redundant code and let
>>      ServerLoop handle it.
>
> I'm going to work a little more on the comments on this one before
> committing; I had just moved all the "If we have lost the XXX, try to
> start a new one" comments as is, but they look pretty repetitive now.

Pushed this now, after adjusting the comments a bit. Thanks again for
the review!

Here are the remaining patches, rebased.

> commit a1c43d65907d20a999b203e465db1277ec842a0a
> Author: Heikki Linnakangas <heikki(dot)linnakangas(at)iki(dot)fi>
> Date: Thu Aug 1 17:24:12 2024 +0300
>
> Introduce a separate BackendType for dead-end children
>
> And replace postmaster.c's own "backend type" codes with BackendType
>
> XXX: While working on this, many times I accidentally did something
> like "foo |= B_SOMETHING" instead of "foo |= 1 << B_SOMETHING", when
> constructing arguments to SignalSomeChildren or CountChildren, and
> things broke in very subtle ways taking a long time to debug. The old
> constants that were already bitmasks avoided that. Maybe we need some
> macro magic or something to make this less error-prone.

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

2. Another idea is to redefine the BackendType values to be separate
bits, like the current BACKEND_TYPE_* values in postmaster.c:

typedef enum BackendType
{
B_INVALID = 0,

/* Backends and other backend-like processes */
B_BACKEND = 1 << 1,
B_DEAD_END_BACKEND = 1 << 2,
B_AUTOVAC_LAUNCHER = 1 << 3,
B_AUTOVAC_WORKER = 1 << 4,

...
} BackendType;

Then you can use | and & on BackendTypes directly. It makes it less
clear which function arguments are a BackendType and which are a
bitmask, however.

Thoughts, other ideas?

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

Attachment Content-Type Size
v4-0001-Add-test-for-connection-limits.patch text/x-patch 6.4 KB
v4-0002-Add-test-for-dead-end-backends.patch text/x-patch 3.2 KB
v4-0003-Use-an-shmem_exit-callback-to-remove-backend-from.patch text/x-patch 4.9 KB
v4-0004-Introduce-a-separate-BackendType-for-dead-end-chi.patch text/x-patch 14.7 KB
v4-0005-Kill-dead-end-children-when-there-s-nothing-else-.patch text/x-patch 7.5 KB
v4-0006-Assign-a-child-slot-to-every-postmaster-child-pro.patch text/x-patch 64.1 KB
v4-0007-Pass-MyPMChildSlot-as-an-explicit-argument-to-chi.patch text/x-patch 10.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message jian he 2024-08-12 10:03:22 minor comments issue in ResultRelInfo src/include/nodes/execnodes.h
Previous Message Arseny Sher 2024-08-12 09:41:55 Taking into account syncrep position in flush_lsn reported by apply worker