Re: dynamic background workers

Hi,

On Sat, Jun 15, 2013 at 6:00 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> The first patch, max-worker-processes-v1.patch, adds a new GUC
> max_worker_processes, which defaults to 8. This fixes the problem
> discussed here:
>
> http://www.postgresql.org/message-id/CA+TgmobguVO+qHnHvxBA2TFkDhw67Y=4Bp405FVABEc_EtO4VQ@mail.gmail.com
>
> Apart from fixing that problem, it's a pretty boring patch.
I just had a look at the first patch which is pretty simple before
looking in details at the 2nd patch.

Here are some minor comments:
1) Correction of postgresql.conf.sample
Putting the new parameter in the section resource usage is adapted,
however why not adding a new sub-section of this type with some
comments like below?
# - Background workers -
#max_worker_processes = 8 # Maximum number of background worker subprocesses
# (change requires restart)
2) Perhaps it would be better to specify in the docs that if the
number of bgworker that are tried to be started by server is higher
than max_worker_processes, startup of the extra bgworkers will fail
but server will continue running as if nothing happened. This is
something users should be made aware of.
3) In InitProcGlobal:proc.c, wouldn't it be more consistent to do that
when assigning new slots in PGPROC:
else if (i < MaxConnections + autovacuum_max_workers + max_worker_processes + 1)
{
/* PGPROC for bgworker, add to bgworkerFreeProcs list */
procs[i].links.next = (SHM_QUEUE *) ProcGlobal->bgworkerFreeProcs;
ProcGlobal->bgworkerFreeProcs = &procs[i];
}
instead of that?
else if (i < MaxBackends)
{
/* PGPROC for bgworker, add to bgworkerFreeProcs list */
procs[i].links.next = (SHM_QUEUE *) ProcGlobal->bgworkerFreeProcs;
ProcGlobal->bgworkerFreeProcs = &procs[i];
}

I have also done many tests with worker_spi and some home-made
bgworkers and the patch is working as expected, the extra bgworkers
are not started once the maximum number set it reached.
I'll try to look at the other patch soon, but I think that the real
discussion on the topic is just beginning... Btw, IMHO, this first
patch can safely be committed as we would have a nice base for future
discussions/reviews.
Regards,
--
Michael