Re: Review: Extra Daemons / bgworker

On 11/28/2012 03:51 PM, Alvaro Herrera wrote:
> I remember your patchset. I didn't look at it for this round, for no
> particular reason. I did look at KaiGai's submission from two
> commitfests ago, and also at a patch from Simon which AFAIK was never
> published openly. Simon's patch merged the autovacuum code to make
> autovac workers behave like bgworkers as handled by his patch, just like
> you suggest. To me it didn't look all that good; I didn't have the guts
> for that, hence the separation. If later bgworkers are found to work
> well enough, we can "port" autovac workers to use this framework, but
> for now it seems to me that the conservative thing is to leave autovac
> untouched.

Hm.. interesting to see how the same idea grows into different patches
and gets refined until we end up with something considered committable.

Do you remember anything in particular that didn't look good? Would you
help reviewing a patch on top of bgworker-7 that changed autovacuum into
a bgworker?

> (As an example, we introduced "ilist" some commits back and changed some
> uses to it; but there are still many places where we're using SHM_QUEUE,
> or List, or open-coded lists, which we could port to the new
> infrastructure, but there's no pressing need to do it.)

Well, I usually like cleaning things up earlier rather than later (my
desk clearly being an exception to that rule, though). But yeah, new
code usually brings new bugs with it.

> Sorry about that -- forgot to push to github. bgworker-7 corresponds to
> commit 0a49a540b which I have just pushed to github.

Thanks.

> Maybe one thing to do in this area would be to ensure that there is a
> certain number of PGPROC elements reserved specifically for bgworkers;
> kind of like autovacuum workers have. That would avoid having regular
> clients exhausting slots for bgworkers, and vice versa.

Yeah, I think that's mandatory, anyways, see below.

> How are you envisioning that the requests would occur?

Just like av_launcher does it now: set a flag in shared memory and
signal the postmaster (PMSIGNAL_START_AUTOVAC_WORKER).

(That's why I'm so puzzled: it looks like it's pretty much all there,
already. I even remember a discussion about that mechanism probably not
being fast enough to spawn bgworkers. And a proposal to add multiple
such flags, so an avlauncher-like daemon could ask for multiple
bgworkers to be started in parallel. I've even measured the serial
bgworker fork rate back then, IIRC it was in the hundreds of forks per
second...)

>> # Minor technical issues or questions
>>
>> In assign_maxconnections, et al, GetNumRegisteredBackgroundWorkers() is
>> used in relation to MAX_BACKENDS or to calculate MaxBackends. That seems
>> to "leak" the bgworkers that registered with neither
>> BGWORKER_SHMEM_ACCESS nor BGWORKER_BACKEND_DATABASE_CONNECTION set. Or
>> is there any reason to discount such extra daemon processes?
>
> No, I purposefully let those out, because those don't need a PGPROC. In
> fact that seems to me to be the whole point of non-shmem-connected
> workers: you can have as many as you like and they won't cause a
> backend-side impact. You can use such a worker to connect via libpq to
> the server, for example.

Hm.. well, in that case, the shmem-connected ones are *not* counted. If
I create and run an extensions that registers 100 shmem-connected
bgworkers, I cannot connect to a system with max_connections=100
anymore, because bgworkers then occupy all of the connections, already.

Please add the registered shmem-connected bgworkers to the
max_connections limit. I think it's counter intuitive to have autovacuum
workers reserved separately, but extension's bgworkers eat (client)
connections. Or put another way: max_connections should always be the
max number of *client* connections the DBA wants to allow.

(Or, if that's in some way complicated, please give the DBA an
additional GUC akin to max_background_workers. That can be merged with
the current max_autovacuum_workers, once/if we rebase autovaccum onto
bgworkers).

>> The additional contrib modules auth_counter and worker_spi are missing
>> from the contrib/Makefile. If that's intentional, they should probably
>> be listed under "Missing".
>>
>> The auth_counter module leaves worker.bgw_restart_time uninitialized.
>>
>> Being an example, it might make sense for auth_counter to provide a
>> signal that just calls SetLatch() to interrupt WaitLatch() and make
>> auth_counter emit its log line upon request, i.e. show how to use SIGUSR1.
>
> KaiGai proposed that we remove auth_counter. I don't see why not; I
> mean, worker_spi is already doing most of what auth_counter is doing, so
> why not?

Agreed.

> However, as you say, maybe we need more coding examples.

Maybe a minimally usable extra daemon example? Showing how to avoid
common pitfalls? Use cases, anybody? :-)

> Ah, that's just a bug, of course.

I see. Glad my review found it.

Regards

Markus Wanner