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

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)