Re: Managing IO workers in postmaster's state machine - Mailing list pgsql-hackers

Hi Andres,

Thanks for sharing these detailed questions and insights. Below are some 
comments and additional thoughts that might help, particularly regarding 
bgworkers.


On 09/12/2024 22:42, Andres Freund wrote:

> Hi,
>
> For AIO, when using the worker process based "AIO", we need to manage a set of
> workers. These need to be started by postmaster, obviously.
>
> I have a few questions about the design details. They seemed mostly
> independent of the larger AIO thread, so I started this separately.
>
>
> 1) Where to start/stop workers after a config reload?
>
> The GUC that controls how many workers are allowed is PGC_SIGHUP. Therefore we
> need to adjust the number of workers after a config reload.
>
> Right now this is implemented (I think by Thomas, but it could have been me),
> in a GUC assign hook. But that seems rather iffy.
>
> For example, I think this means that, as we're in the middle of the GUC reload
> process, a child forked in that moment may have some updated GUCs, but not
> others.  But even besides that, it "feels wrong", in a way I can't fully
> articulate.
>
> However, it's not exactly obvious *where* workers should be started / stopped
> instead.
>
> One approach would be to put the adjustment in PostmasterStateMachine(), but
> currently a config reload request doesn't trigger an invocation of
> PostmasterStateMachine().
>
> We have a somewhat similar case for bgworkers, for those we have a dedicated
> call in ServerLoop().  I guess we could just copy that, but it doesn't seem
> exactly pretty.


I am unsure why background workers are managed directly in the 
ServerLoop(), wouldn't it help if a "background worker launcher" exist 
instead ?

> 2) New state machine phase for AIO shutdown?
>
> During non-crash shutdown, we currently stop archiver and walsender last
> (except dead-end children, but those don't matter in this context). But at
> least walsender might use AIO, so we need to shut down AIO after walsender.
>
> We already have PM_SHUTDOWN, PM_SHUTDOWN_2. I didn't want to introduce
> PM_SHUTDOWN_3. For now there's a new PM_SHUTDOWN_IO.
>
> Is that the right approach?
>
> But somehow the number of shutdown phases seems to be getting out of control,
> besides PM_SHUTDOWN* we also have PM_STOP_BACKENDS, PM_WAIT_BACKENDS,
> PM_WAIT_DEAD_END and PM_NO_CHILDREN...


For the startup there is a StartupStatusEnum, it serves distinct purpose 
but maybe having ShutdownSequence/ShutdownStatus can be helpful: just 
outline order of operations for the shutdown state ? Maybe nearest from 
a chain of responsability than the existing state machine...

> 3) Do we need an new PM* phase at startup?
>
> Because the startup process - obviously - can perform IO, IO workers need to
> be started before the startup process is started.
>
> We already do this with checkpointer and bgwriter. For those we don't have a
> new phase, we just do so before forking the startup process. That means that
> the startup process might need to wait for checkpointer to finish starting up,
> but that seems fine to me.  The same seems true for IO workers.
>
> Therefore I don't see a need for a new phase?

So it should belong to PM_INIT? but the next question:

> 4) Resetting pmState during crash-restart?
>
> For the IO worker handling we need to check the phase we currently are in to
> know whether to start new workers (so we don't start more workers while
> shutting down).
>
> That's easy during normal startup, the state is PM_INIT. But for a
> crash-restart (bottom of PostmasterStateMachine()), pmState will be
> PM_NO_CHILDREN - in which we would *NOT* want to start new IO workers normally
> (if e.g. a SIGHUP is processed while in that state). But we do need to start
> IO workers at that point in the crash-restart cycle.
>
> The AIO patchset has dealt with that by moving pmState = PM_STARTUP to a bit
> earlier. But that seems a *bit* weird, even though I don't see any negative
> consequences right now.
>
> Would it be better to reset pmState to PM_INIT? That doesn't seem quite right
> either, it's described as "postmaster starting".

The comments in postmaster.c said that crash recovery is like shutdown 
followed by startup (not by init).
If I understood correctly IO workers will need to be restarted in such 
context, so they must be after PM_INIT ...


> 5) btmask_all_except2() not used anymore, btmask_all_except3() needed
>
> The one place using btmask_all_except2() now also needs to allow IO workers,
> and thus btmask_all_except3(). Which triggers warnings about
> btmask_all_except2 being unused.
>
> We could remove btmask_all_except2(), ifdef it out or create a more generic
> version that works with arbitrary numbers of arguments.
>
> Something like
>
> static inline BackendTypeMask
> btmask_all_except_n(int nargs, BackendType *t)
> {
>      BackendTypeMask mask = BTYPE_MASK_ALL;
>
>      for (int i = 0; i < nargs; i++)
>          mask = btmask_del(mask, t[i]);
>      return mask;
> }
>
> #define btmask_all_except(...) \
>      btmask_all_except_n( \
>          lengthof(((BackendType[]){__VA_ARGS__})), \
>          (BackendType[]){__VA_ARGS__} \
>      )
>
> should do the trick, I think.
>
>
> 6) Variable numbered aux processes
>
> The IO workers are aux processes - that seemed to be the most
> appropriate. However, if "real" AIO is configured (PGC_POSTMASTER), no IO
> workers can be started.
>
> The only change the patch had to make for that is:
>
> -#define NUM_AUXILIARY_PROCS     6
> +#define MAX_IO_WORKERS          32
> +#define NUM_AUXILIARY_PROCS     (6 + MAX_IO_WORKERS)
>
> Which means that we'll reserve some resources for IO workers even if no IO
> workers can be started. Do we consider that a problem?  There's some precedent
> - we reserve space for archiver even if not configured, despite archive_mode
> being PGC_POSTMASTER - but of course that's a different scale.


I have not checked the history, I have read some details on this topic, 
but the reasoning is not yet very clear to me as **why** this is 
hardcoded.  If there's a technical or historical reason for the current 
design, it would be useful to revisit that in this context.
What prevent the number of background workers to be dynamically sized 
today ?

---
Cédric Villemain +33 6 20 30 22 52
https://www.Data-Bene.io
PostgreSQL Support, Expertise, Training, R&D