Thread

1. Review: Extra Daemons / bgworker

   Markus Wanner <markus@bluegap.ch> — 2012-11-28T13:57:26Z

   Hello Álvaro,
   
   first of all, thank you for bringing this up again and providing a
   patch. My first attempt on that was more than two years ago [1]. As the
   author of a former bgworker patch, I'd like to provide an additional
   review - KaiGai was simply faster to sing up as a reviewer on the
   commitfest app...
   
   I started my review is based on rev 6d7e51.. from your bgworker branch
   on github, only to figure out later that it wasn't quite up to date. I
   upgraded to bgworker-7.patch. I hope that's the most recent version.
   
   
   # Concept
   
   I appreciate the ability to run daemons that are not connected to
   Postgres shared memory. It satisfies a user request that came up several
   times when I talked about the bgworkers in Postgres-R.
   
   Another good point is the flexible interface via extensions, even
   allowing different starting points for such background workers.
   
   One thing I'd miss (for use of that framework in Postgres-R) is the
   ability to start a registered bgworker only upon request, way after the
   system started. So that the bgworker process doesn't even exist until it
   is really needed. I realize that RegisterBackgroundWorker() is still
   necessary in advance to reserve the slots in BackgroundWorkerList and
   shared memory.
   
   As a use case independent of Postgres-R, think of something akin to a
   worker_spi, but wanting that to perform a task every 24h on hundreds of
   databases. You don't want to keep hundreds of processes occupying PGPROC
   slots just perform a measly task every 24h.
   
   (We've discussed the ability to let bgworkers re-connect to another
   database back then. For one, you'd still have currently unneeded worker
   processes around all the time. And second, I think that's hard to get
   right - after all, a terminated process is guaranteed to not leak any
   stale data into a newly started one, no matter what.)
   
   From my point of view, autovacuum is the very first example of a
   background worker process. And I'm a bit puzzled about it not being
   properly integrated into this framework. Once you think of autovacuum as
   a background job which needs access to Postgres shared memory and a
   database, but no client connection, it looks like a bit of code
   duplication (and not using code we already have). I realize this kind of
   needs the above feature being able to request the (re)start of bgworkers
   at arbitrary points in time. However, it would also be a nice test case
   for the bgworker infrastructure.
   
   I'd be happy to help with extending the current patch into that
   direction, if you agree it's generally useful. Or adapt my bgworker code
   accordingly.
   
   
   # 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?
   
   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.
   
   The bgw_restart_time doesn't always work (certainly not the way I'm
   expecting it to). For example, if you forget to pass the
   BGWORKER_SHMEM_ACCESS flag, trying to LWLockAcquire() leads to the
   worker being restarted immediately and repeatedly - independent of the
   bgw_restart_time setting. The same holds true if the bgworker exits with
   status 0 or in case it segfaults. Not when exiting with code 1, though.
   Why is that? Especially in case of a segfault or equally "hard" errors
   that can be expected to occur repeatedly, I don't want the worker to be
   restarted that frequently.
   
   
   # Documentation
   
   There are two working examples in contrib. The auth_counter could use a
   header comment similar to worker_spi, quickly describing what it does.
   There's no example of a plain extra daemon, without shmem access.
   
   Coding guidelines for bgworker / extra daemon writers are missing. I
   read these must not use sleep(), with an explanation in both examples.
   Other questions that come to mind: what about signal handlers? fork()?
   threads? Can/should it use PG_TRY/PG_CATCH or setjmp()/longjmp(). How to
   best load other 3rd party libraries, i.e. for OpenGL processing?
   
   Especially for extra daemons (i.e. bgw_flags = 0), differences to
   running an external daemon should be documented. I myself am unclear
   about all of the implications that running as a child of the postmaster
   has (OOM and cgroups come to mind - there certainly are other aspects).
   (I myself still have a hard time finding a proper use case for extra
   daemons. I don't want the postmaster to turn into a general purpose
   watchdog for everything and the kitchen sink.)
   
   
   # Tests
   
   No additional automated tests are included - hard to see how that could
   be tested automatically, given the lack of a proper test harness.
   
   
   I hope this review provides useful input.
   
   Regards
   
   Markus Wanner
   
   
   [1]: That was during commit fest 2010-09. Back then, neither extensions,
   latches nor the walsender existed. The patches had a dependency on
   imessages, which was not accepted. Other than that, it's a working,
   pooled bgworker implementation on top of which autovacuum runs just
   fine. It lacks extensibility and the extra daemons feature, though. For
   those who missed it:
   
   https://commitfest.postgresql.org/action/patch_view?id=339
   http://archives.postgresql.org/message-id/4C3C789C.1040409@bluegap.ch
   http://git.postgres-r.org/?p=bgworker;a=summary
   
   
   

2. Re: Review: Extra Daemons / bgworker

   Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-11-28T14:51:18Z

   Markus Wanner wrote:
   
   Hi Markus,
   
   Many thanks for your review.
   
   > first of all, thank you for bringing this up again and providing a
   > patch. My first attempt on that was more than two years ago [1]. As the
   > author of a former bgworker patch, I'd like to provide an additional
   > review - KaiGai was simply faster to sing up as a reviewer on the
   > commitfest app...
   
   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.
   
   (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.)
   
   > I started my review is based on rev 6d7e51.. from your bgworker branch
   > on github, only to figure out later that it wasn't quite up to date. I
   > upgraded to bgworker-7.patch. I hope that's the most recent version.
   
   Sorry about that -- forgot to push to github.  bgworker-7 corresponds to
   commit 0a49a540b which I have just pushed to github.
   
   > # Concept
   > 
   > I appreciate the ability to run daemons that are not connected to
   > Postgres shared memory. It satisfies a user request that came up several
   > times when I talked about the bgworkers in Postgres-R.
   > 
   > Another good point is the flexible interface via extensions, even
   > allowing different starting points for such background workers.
   
   Great.
   
   > One thing I'd miss (for use of that framework in Postgres-R) is the
   > ability to start a registered bgworker only upon request, way after the
   > system started. So that the bgworker process doesn't even exist until it
   > is really needed. I realize that RegisterBackgroundWorker() is still
   > necessary in advance to reserve the slots in BackgroundWorkerList and
   > shared memory.
   > 
   > As a use case independent of Postgres-R, think of something akin to a
   > worker_spi, but wanting that to perform a task every 24h on hundreds of
   > databases. You don't want to keep hundreds of processes occupying PGPROC
   > slots just perform a measly task every 24h.
   
   Yeah, this is something I specifically kept out initially to keep things
   simple.
   
   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.
   
   How are you envisioning that the requests would occur?
   
   > # 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.
   
   > 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?  However, as you say, maybe we need more coding examples.
   
   > The bgw_restart_time doesn't always work (certainly not the way I'm
   > expecting it to). For example, if you forget to pass the
   > BGWORKER_SHMEM_ACCESS flag, trying to LWLockAcquire() leads to the
   > worker being restarted immediately and repeatedly - independent of the
   > bgw_restart_time setting. The same holds true if the bgworker exits with
   > status 0 or in case it segfaults. Not when exiting with code 1, though.
   > Why is that? Especially in case of a segfault or equally "hard" errors
   > that can be expected to occur repeatedly, I don't want the worker to be
   > restarted that frequently.
   
   Ah, that's just a bug, of course.
   
   -- 
   Álvaro Herrera                http://www.2ndQuadrant.com/
   PostgreSQL Development, 24x7 Support, Training & Services
   
   
   

3. Re: Review: Extra Daemons / bgworker

   Markus Wanner <markus@bluegap.ch> — 2012-11-28T15:45:51Z

   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
   
   
   

4. Re: Review: Extra Daemons / bgworker

   Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2012-11-30T10:09:29Z

   Markus Wanner <markus@bluegap.ch> writes:
   >> 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? :-)
   
   What about the PGQ ticker, pgqd?
   
     https://github.com/markokr/skytools/tree/master/sql/ticker
     https://github.com/markokr/skytools/blob/master/sql/ticker/pgqd.c
   
   Or maybe pgAgent, which seems to live there, but is in C++ so might need
   a rewrite to the specs:
   
     http://git.postgresql.org/gitweb/?p=pgadmin3.git;a=tree;f=pgadmin/agent;h=ebbcf71bd918efdc82466785ffac6f2ac3443847;hb=HEAD
   
   Maybe it would be easier to have a version of GNU mcron as an extension,
   with the abitity to fire PostgreSQL stored procedures directly?  (That
   way the cron specific parts of the logic are already implemented)
   
     http://www.gnu.org/software/mcron/
   
   Another idea would be to have a pgbouncer extension. We would still need
   of course to have pgbouncer as a separate component so that client
   connection can outlive a postmaster crash, but that would still be very
   useful as a first step into admission control. Let's not talk about the
   feedback loop and per-cluster resource usage monitoring yet, but I guess
   that you can see the drift.
   
   Regards,
   -- 
   Dimitri Fontaine
   http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support
   
   
   

5. Re: Review: Extra Daemons / bgworker

   Pavel Stehule <pavel.stehule@gmail.com> — 2012-11-30T10:15:47Z

   2012/11/30 Dimitri Fontaine <dimitri@2ndquadrant.fr>:
   > Markus Wanner <markus@bluegap.ch> writes:
   >>> 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? :-)
   >
   > What about the PGQ ticker, pgqd?
   >
   >   https://github.com/markokr/skytools/tree/master/sql/ticker
   >   https://github.com/markokr/skytools/blob/master/sql/ticker/pgqd.c
   >
   > Or maybe pgAgent, which seems to live there, but is in C++ so might need
   > a rewrite to the specs:
   >
   >   http://git.postgresql.org/gitweb/?p=pgadmin3.git;a=tree;f=pgadmin/agent;h=ebbcf71bd918efdc82466785ffac6f2ac3443847;hb=HEAD
   >
   > Maybe it would be easier to have a version of GNU mcron as an extension,
   > with the abitity to fire PostgreSQL stored procedures directly?  (That
   > way the cron specific parts of the logic are already implemented)
   >
   >   http://www.gnu.org/software/mcron/
   >
   > Another idea would be to have a pgbouncer extension. We would still need
   > of course to have pgbouncer as a separate component so that client
   > connection can outlive a postmaster crash, but that would still be very
   > useful as a first step into admission control. Let's not talk about the
   > feedback loop and per-cluster resource usage monitoring yet, but I guess
   > that you can see the drift.
   
   both will be nice
   
   Pavel
   
   >
   > Regards,
   > --
   > Dimitri Fontaine
   > http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support
   >
   >
   > --
   > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
   > To make changes to your subscription:
   > http://www.postgresql.org/mailpref/pgsql-hackers
   
   
   

6. Re: Review: Extra Daemons / bgworker

   Markus Wanner <markus@bluegap.ch> — 2012-11-30T12:06:13Z

   On 11/30/2012 11:09 AM, Dimitri Fontaine wrote:
   > Markus Wanner <markus@bluegap.ch> writes:
   >>> 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? :-)
   > 
   > What about the PGQ ticker, pgqd?
   > 
   >   https://github.com/markokr/skytools/tree/master/sql/ticker
   >   https://github.com/markokr/skytools/blob/master/sql/ticker/pgqd.c
   
   AFAICS pgqd currently uses libpq, so I think it would rather turn into
   what I call a background worker, with a connection to Postgres shared
   memory. I perfectly well see use cases (plural!) for those.
   
   What I'm questioning is the use for what I rather call "extra daemons",
   that is, additional processes started by the postmaster without a
   connection to Postgres shared memory (and thus without a database
   connection).
   
   I was asking for a minimal example of such an extra daemon, similar to
   worker_spi, showing how to properly write such a thing. Ideally showing
   how to avoid common pitfalls.
   
   > Maybe it would be easier to have a version of GNU mcron as an extension,
   > with the abitity to fire PostgreSQL stored procedures directly?  (That
   > way the cron specific parts of the logic are already implemented)
   > 
   >   http://www.gnu.org/software/mcron/
   
   Again, that's something that would eventually require a database
   connection. Or at least access to Postgres shared memory to eventually
   start the required background jobs. (Something Alvaro's patch doesn't
   implement, yet. But which exactly matches what the coordinator and
   bgworkers in Postgres-R do.)
   
   For ordinary extra daemons, I'm worried about things like an OOM killer
   deciding to kill the postmaster, being its parent. Or (io)niceness
   settings. Or Linux cgroups. IMO all of these things just get harder to
   administrate for extra daemons, when they move under the hat of the
   postmaster.
   
   Without access to shared memory, the only thing an extra daemon would
   gain by moving under postmaster is the "knowledge" of the start, stop
   and restart (crash) events of the database. And that in a very
   inflexible way: the extra process is forced to start, stop and restart
   together with the database to "learn" about these events.
   
   Using a normal client connection arguably is a better way to learn about
   crash events - it doesn't have the above limitation. And the start and
   stop events, well, the DBA or sysadmin is under control of these,
   already. We can possibly improve on that, yes. But extra daemons are not
   the way to go, IMO.
   
   > Another idea would be to have a pgbouncer extension. We would still need
   > of course to have pgbouncer as a separate component so that client
   > connection can outlive a postmaster crash, but that would still be very
   > useful as a first step into admission control. Let's not talk about the
   > feedback loop and per-cluster resource usage monitoring yet, but I guess
   > that you can see the drift.
   
   Sorry, I don't. Especially not with something like pgbouncer, because I
   definitely *want* to control and administrate that separately.
   
   So I guess this is a vote to disallow `worker.bgw_flags = 0` on the
   basis that everything a process, which doesn't need to access Postgres
   shared memory, better does whatever it does outside of Postgres. For the
   benefit of the stability of Postgres and for ease of administration of
   the two.
   
   Or, maybe, rather drop the BGWORKER_SHMEM_ACCESS flag and imply that
   every registered process wants to have access to Postgres shared memory.
   Then document the gotchas and requirements of living under the
   Postmaster, as I've requested before. (If you want a foot gun, you can
   still write an extension that doesn't need to access Postgres shared
   memory, but hey.. I we can't help those who desperately try to shoot
   their foot).
   
   Regards
   
   Markus Wanner
   
   
   

7. Re: Review: Extra Daemons / bgworker

   Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2012-11-30T12:40:35Z

   Markus Wanner <markus@bluegap.ch> writes:
   > AFAICS pgqd currently uses libpq, so I think it would rather turn into
   > what I call a background worker, with a connection to Postgres shared
   > memory. I perfectly well see use cases (plural!) for those.
   >
   > What I'm questioning is the use for what I rather call "extra daemons",
   > that is, additional processes started by the postmaster without a
   > connection to Postgres shared memory (and thus without a database
   > connection).
   
   I totally missed the need to connect to shared memory to be able to
   connect to a database and query it. Can't we just link the bgworkder
   code to the client libpq library, just as plproxy is doing I believe?
   
   Regards,
   -- 
   Dimitri Fontaine
   http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support
   
   
   

8. Re: Review: Extra Daemons / bgworker

   Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-11-30T12:57:20Z

   Dimitri Fontaine wrote:
   > Markus Wanner <markus@bluegap.ch> writes:
   > > AFAICS pgqd currently uses libpq, so I think it would rather turn into
   > > what I call a background worker, with a connection to Postgres shared
   > > memory. I perfectly well see use cases (plural!) for those.
   > >
   > > What I'm questioning is the use for what I rather call "extra daemons",
   > > that is, additional processes started by the postmaster without a
   > > connection to Postgres shared memory (and thus without a database
   > > connection).
   > 
   > I totally missed the need to connect to shared memory to be able to
   > connect to a database and query it. Can't we just link the bgworkder
   > code to the client libpq library, just as plproxy is doing I believe?
   
   One of the uses for bgworkers that don't have shmem connection is to
   have them use libpq connections instead.  I don't really see the point
   of forcing everyone to use backend connections when libpq connections
   are enough.  In particular, they are easier to port from existing code;
   and they make it easier to share code with systems that still have to
   support older PG versions.
   
   -- 
   Álvaro Herrera                http://www.2ndQuadrant.com/
   PostgreSQL Development, 24x7 Support, Training & Services
   
   
   

9. Re: Review: Extra Daemons / bgworker

   Andres Freund <andres@2ndquadrant.com> — 2012-11-30T12:59:15Z

   On 2012-11-30 09:57:20 -0300, Alvaro Herrera wrote:
   > Dimitri Fontaine wrote:
   > > Markus Wanner <markus@bluegap.ch> writes:
   > > > AFAICS pgqd currently uses libpq, so I think it would rather turn into
   > > > what I call a background worker, with a connection to Postgres shared
   > > > memory. I perfectly well see use cases (plural!) for those.
   > > >
   > > > What I'm questioning is the use for what I rather call "extra daemons",
   > > > that is, additional processes started by the postmaster without a
   > > > connection to Postgres shared memory (and thus without a database
   > > > connection).
   > >
   > > I totally missed the need to connect to shared memory to be able to
   > > connect to a database and query it. Can't we just link the bgworkder
   > > code to the client libpq library, just as plproxy is doing I believe?
   >
   > One of the uses for bgworkers that don't have shmem connection is to
   > have them use libpq connections instead.  I don't really see the point
   > of forcing everyone to use backend connections when libpq connections
   > are enough.  In particular, they are easier to port from existing code;
   > and they make it easier to share code with systems that still have to
   > support older PG versions.
   
   They also can get away with a lot more crazy stuff without corrupting
   the database. You better know something about what youre doing before
   doing something with direct shared memory access.
   
   Greetings,
   
   Andres Freund
   
   --
    Andres Freund	                   http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
   
   
   

10. Re: Review: Extra Daemons / bgworker

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2012-11-30T13:03:57Z

    Andres Freund <andres@2ndquadrant.com> writes:
    >> One of the uses for bgworkers that don't have shmem connection is to
    >> have them use libpq connections instead.  I don't really see the point
    >> of forcing everyone to use backend connections when libpq connections
    >> are enough.  In particular, they are easier to port from existing code;
    >> and they make it easier to share code with systems that still have to
    >> support older PG versions.
    
    Exactly, I think most bgworker would just use libpq if that's available,
    using a backend's infrastructure is not that good a fit here. I mean,
    connect from your worker to a database using libpq and call a backend's
    function (provided by the same extension I guess) in there.
    
    That's how I think pgqd would get integrated into the worker
    infrastructure, right?
    
    > They also can get away with a lot more crazy stuff without corrupting
    > the database. You better know something about what youre doing before
    > doing something with direct shared memory access.
    
    And there's a whole lot you can already do just with a C coded stored
    procedure already.
    
    Regards,
    -- 
    Dimitri Fontaine
    http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support
    
    
    

11. Re: Review: Extra Daemons / bgworker

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-11-30T13:21:49Z

    2012/11/30 Dimitri Fontaine <dimitri@2ndquadrant.fr>:
    > Andres Freund <andres@2ndquadrant.com> writes:
    >>> One of the uses for bgworkers that don't have shmem connection is to
    >>> have them use libpq connections instead.  I don't really see the point
    >>> of forcing everyone to use backend connections when libpq connections
    >>> are enough.  In particular, they are easier to port from existing code;
    >>> and they make it easier to share code with systems that still have to
    >>> support older PG versions.
    >
    > Exactly, I think most bgworker would just use libpq if that's available,
    > using a backend's infrastructure is not that good a fit here. I mean,
    > connect from your worker to a database using libpq and call a backend's
    > function (provided by the same extension I guess) in there.
    >
    > That's how I think pgqd would get integrated into the worker
    > infrastructure, right?
    >
    One thing we have to pay attention is, the backend code cannot distinguish
    connection from pgworker via libpq from other regular connections, from
    perspective of access control.
    Even if we implement major portion with C-function, do we have a reliable way
    to prohibit C-function being invoked with user's query?
    
    I also plan to use bgworker to load data chunk from shared_buffer to GPU
    device in parallel, it shall be performed under the regular access control
    stuff.
    
    I think, using libpq is a good "option" for 95% of development, however, it
    also should be possible to use SPI interface for corner case usage.
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    
    

12. Re: Review: Extra Daemons / bgworker

    Markus Wanner <markus@bluegap.ch> — 2012-11-30T13:23:02Z

    On 11/30/2012 01:40 PM, Dimitri Fontaine wrote:
    > I totally missed the need to connect to shared memory to be able to
    > connect to a database and query it. Can't we just link the bgworkder
    > code to the client libpq library, just as plproxy is doing I believe?
    
    Well, sure you can use libpq. But so can external processes. So that's
    no benefit of running under the postmaster.
    
    Regards
    
    Markus Wanner
    
    
    

13. Re: Review: Extra Daemons / bgworker

    Markus Wanner <markus@bluegap.ch> — 2012-11-30T13:31:54Z

    On 11/30/2012 01:59 PM, Andres Freund wrote:
    > On 2012-11-30 09:57:20 -0300, Alvaro Herrera wrote:
    >> One of the uses for bgworkers that don't have shmem connection is to
    >> have them use libpq connections instead.  I don't really see the point
    >> of forcing everyone to use backend connections when libpq connections
    >> are enough.
    
    Requiring a libpq connection is a good indication for *not* wanting the
    process to run under the postmaster, IMO.
    
    >> In particular, they are easier to port from existing code;
    >> and they make it easier to share code with systems that still have to
    >> support older PG versions.
    > 
    > They also can get away with a lot more crazy stuff without corrupting
    > the database.
    
    Exactly. That's a good reason to *not* tie that to the postmaster, then.
    Please keep as much of the potentially dangerous stuff separate (and
    advice developers to do so as well, instead of offering them a foot
    gun). So that our postmaster can do its job. And do it reliably, without
    trying to be a general purpose start/stop daemon. There are better and
    well established tools for that.
    
    Regards
    
    Markus Wanner
    
    
    

14. Re: Review: Extra Daemons / bgworker

    Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2012-11-30T13:42:55Z

    Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
    > One thing we have to pay attention is, the backend code cannot distinguish
    > connection from pgworker via libpq from other regular connections, from
    > perspective of access control.
    > Even if we implement major portion with C-function, do we have a reliable way
    > to prohibit C-function being invoked with user's query?
    
    Why would you want to do that? And maybe the way to enforce that would
    be by having your extension do its connecting using SPI to be able to
    place things in known pieces of memory for the function to check?
    
    > I also plan to use bgworker to load data chunk from shared_buffer to GPU
    > device in parallel, it shall be performed under the regular access control
    > stuff.
    
    That sounds like a job where you need shared memory access but maybe not
    a database connection?
    
    > I think, using libpq is a good "option" for 95% of development, however, it
    > also should be possible to use SPI interface for corner case usage.
    
    +1, totally agreed.
    
    -- 
    Dimitri Fontaine
    http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support
    
    
    

15. Re: Review: Extra Daemons / bgworker

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-11-30T13:44:43Z

    Markus Wanner wrote:
    > On 11/30/2012 01:40 PM, Dimitri Fontaine wrote:
    > > I totally missed the need to connect to shared memory to be able to
    > > connect to a database and query it. Can't we just link the bgworkder
    > > code to the client libpq library, just as plproxy is doing I believe?
    > 
    > Well, sure you can use libpq. But so can external processes. So that's
    > no benefit of running under the postmaster.
    
    No, it's not a benefit of that; but such a process would get started up
    when postmaster is started, and shut down when postmaster stops.  So it
    makes easier to have processes that need to run alongside postmaster.
    
    -- 
    Álvaro Herrera                http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

16. Re: Review: Extra Daemons / bgworker

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-11-30T13:55:33Z

    2012/11/30 Dimitri Fontaine <dimitri@2ndquadrant.fr>:
    > Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
    >> One thing we have to pay attention is, the backend code cannot distinguish
    >> connection from pgworker via libpq from other regular connections, from
    >> perspective of access control.
    >> Even if we implement major portion with C-function, do we have a reliable way
    >> to prohibit C-function being invoked with user's query?
    >
    > Why would you want to do that? And maybe the way to enforce that would
    > be by having your extension do its connecting using SPI to be able to
    > place things in known pieces of memory for the function to check?
    >
    As long as SPI is an option of bgworker, I have nothing to argue here.
    If the framework enforced extension performing as background worker using
    libpq for database connection, a certain kind of works being tied with internal
    stuff has to be implemented as C-functions. Thus, I worried about it.
    
    >> I also plan to use bgworker to load data chunk from shared_buffer to GPU
    >> device in parallel, it shall be performed under the regular access control
    >> stuff.
    >
    > That sounds like a job where you need shared memory access but maybe not
    > a database connection?
    >
    Both of them are needed in this case. This "io-worker" will perform according
    to the request from regular backend process, and fetch tuples from the heap
    to the DMA buffer being on shared memory.
    
    >> I think, using libpq is a good "option" for 95% of development, however, it
    >> also should be possible to use SPI interface for corner case usage.
    >
    > +1, totally agreed.
    >
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    
    

17. Re: Review: Extra Daemons / bgworker

    Markus Wanner <markus@bluegap.ch> — 2012-11-30T13:55:48Z

    Alvaro,
    
    On 11/30/2012 02:44 PM, Alvaro Herrera wrote:
    > So it
    > makes easier to have processes that need to run alongside postmaster.
    
    That's where we are in respectful disagreement, then. As I don't think
    that's easier, overall, but in my eye, this looks like a foot gun.
    
    As long as things like pgbouncer, pgqd, etc.. keep to be available as
    separate daemons, I'm happy, though.
    
    Regards
    
    Markus Wanner
    
    
    

18. Re: Review: Extra Daemons / bgworker

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-11-30T14:16:26Z

    2012/11/30 Markus Wanner <markus@bluegap.ch>:
    > Alvaro,
    >
    > On 11/30/2012 02:44 PM, Alvaro Herrera wrote:
    >> So it
    >> makes easier to have processes that need to run alongside postmaster.
    >
    > That's where we are in respectful disagreement, then. As I don't think
    > that's easier, overall, but in my eye, this looks like a foot gun.
    >
    > As long as things like pgbouncer, pgqd, etc.. keep to be available as
    > separate daemons, I'm happy, though.
    >
    This feature does not enforce them to implement with this new framework.
    If they can perform as separate daemons, it is fine enough.
    
    But it is not all the cases where we want background workers being tied
    with postmaster's duration.
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    
    

19. Re: Review: Extra Daemons / bgworker

    Markus Wanner <markus@bluegap.ch> — 2012-11-30T14:27:47Z

    On 11/30/2012 03:16 PM, Kohei KaiGai wrote:
    > This feature does not enforce them to implement with this new framework.
    > If they can perform as separate daemons, it is fine enough.
    
    I'm not clear on what exactly you envision, but if a process needs
    access to shared buffers, it sounds like it should be a bgworker. I
    don't quite understand why that process also wants a libpq connection,
    but that's certainly doable.
    
    > But it is not all the cases where we want background workers being tied
    > with postmaster's duration.
    
    Not wanting a process to be tied to postmaster's duration is a strong
    indication that it better not be a bgworker, I think.
    
    Regards
    
    Markus Wanner
    
    
    

20. Re: Review: Extra Daemons / bgworker

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-11-30T14:58:57Z

    2012/11/30 Markus Wanner <markus@bluegap.ch>:
    > On 11/30/2012 03:16 PM, Kohei KaiGai wrote:
    >> This feature does not enforce them to implement with this new framework.
    >> If they can perform as separate daemons, it is fine enough.
    >
    > I'm not clear on what exactly you envision, but if a process needs
    > access to shared buffers, it sounds like it should be a bgworker. I
    > don't quite understand why that process also wants a libpq connection,
    > but that's certainly doable.
    >
    It seemed to me you are advocating that any use case of background-
    worker can be implemented with existing separate daemon approach.
    What I wanted to say is, we have some cases that background-worker
    framework allows to implement such kind of extensions with more
    reasonable design.
    
    Yes, one reason I want to use background-worker is access to shared-
    memory segment. Also, it want to connect databases simultaneously
    out of access controls; like as autovacuum. It is a reason why I'm saying
    SPI interface should be only an option, not only libpq.
    (If extension choose libpq, it does not take anything special, does it?)
    
    >> But it is not all the cases where we want background workers being tied
    >> with postmaster's duration.
    >
    > Not wanting a process to be tied to postmaster's duration is a strong
    > indication that it better not be a bgworker, I think.
    >
    It also probably means, in case when a process whose duration wants to
    be tied with duration of postmaster, its author can consider to implement
    it as background worker.
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    
    

21. Re: Review: Extra Daemons / bgworker

    Markus Wanner <markus@bluegap.ch> — 2012-11-30T15:22:32Z

    On 11/30/2012 03:58 PM, Kohei KaiGai wrote:
    > It seemed to me you are advocating that any use case of background-
    > worker can be implemented with existing separate daemon approach.
    
    That sounds like a misunderstanding. All I'm advocating is that only
    3rd-party processes with a real need (like accessing shared memory)
    should run under the postmaster.
    
    > What I wanted to say is, we have some cases that background-worker
    > framework allows to implement such kind of extensions with more
    > reasonable design.
    
    I absolutely agree to that. And I think I can safely claim to be the
    first person to publish a patch that provides some kind of background
    worker infrastructure for Postgres.
    
    > Yes, one reason I want to use background-worker is access to shared-
    > memory segment. Also, it want to connect databases simultaneously
    > out of access controls; like as autovacuum.
    
    Yeah, that's the entire reason for background workers. For clarity and
    differentiation, I'd add: .. without having a client connection. That's
    what makes them *background* workers. (Not to be confused with the
    frontend vs backend differentiation. They are background backends, if
    you want).
    
    > It is a reason why I'm saying
    > SPI interface should be only an option, not only libpq.
    
    I'm extending that to say extensions should better *not* use libpq.
    After all, they have a more direct access, already.
    
    > It also probably means, in case when a process whose duration wants to
    > be tied with duration of postmaster, its author can consider to implement
    > it as background worker.
    
    I personally don't count that as a real need. There are better tools for
    this job; while there clearly are dangers in (ab)using the postmaster to
    do it.
    
    Regards
    
    Markus Wanner
    
    
    

22. Re: Review: Extra Daemons / bgworker

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-12-03T14:28:10Z

    Markus Wanner wrote:
    > 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?
    
    I'm not really that interested in it currently ... and there are enough
    other patches to review.  I would like bgworkers to mature a bit more
    and get some heavy real world testing before we change autovacuum.
    
    > > 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).
    
    I'm not sure how this works.  What process is in charge of setting such
    a flag?
    
    > >> 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.
    
    This is actually a very silly bug: it turns out that guc.c obtains the
    count of workers before workers have actually registered.  So this
    necessitates some reshuffling.
    
    > Or put another way: max_connections should always be the
    > max number of *client* connections the DBA wants to allow.
    
    Completely agreed.
    
    -- 
    Álvaro Herrera                http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

23. Re: Review: Extra Daemons / bgworker

    Markus Wanner <markus@bluegap.ch> — 2012-12-03T15:17:37Z

    On 12/03/2012 03:28 PM, Alvaro Herrera wrote:
    > I'm not really that interested in it currently ... and there are enough
    > other patches to review.  I would like bgworkers to mature a bit more
    > and get some heavy real world testing before we change autovacuum.
    
    Understood.
    
    >> Just like av_launcher does it now: set a flag in shared memory and
    >> signal the postmaster (PMSIGNAL_START_AUTOVAC_WORKER).
    > 
    > I'm not sure how this works.  What process is in charge of setting such
    > a flag?
    
    The only process that currently starts background workers ... ehm ...
    autovacuum workers is the autovacuum launcher. It uses the above
    Postmaster Signal in autovacuum.c:do_start_autovacuum_worker() to have
    the postmaster launch bg/autovac workers on demand.
    
    (And yes, this kind of is an exception to the rule that the postmaster
    must not rely on shared memory. However, these are just flags we write
    atomically, see pmsignal.[ch])
    
    I'd like to extend that, so that other processes can request to start
    (pre-registered) background workers more dynamically. I'll wait for an
    update of your patch, though.
    
    > This is actually a very silly bug: it turns out that guc.c obtains the
    > count of workers before workers have actually registered.  So this
    > necessitates some reshuffling.
    
    Okay, thanks.
    
    Regards
    
    Markus Wanner
    
    
    

24. Re: Review: Extra Daemons / bgworker

    Simon Riggs <simon@2ndquadrant.com> — 2012-12-03T15:27:31Z

    On 3 December 2012 15:17, Markus Wanner <markus@bluegap.ch> wrote:
    
    >>> Just like av_launcher does it now: set a flag in shared memory and
    >>> signal the postmaster (PMSIGNAL_START_AUTOVAC_WORKER).
    >>
    >> I'm not sure how this works.  What process is in charge of setting such
    >> a flag?
    >
    > The only process that currently starts background workers ... ehm ...
    > autovacuum workers is the autovacuum launcher. It uses the above
    > Postmaster Signal in autovacuum.c:do_start_autovacuum_worker() to have
    > the postmaster launch bg/autovac workers on demand.
    
    My understanding was that the patch keep autovac workers and
    background workers separate at this point.
    
    Is there anything to be gained *now* from merging those two concepts?
    I saw that as refactoring that can occur once we are happy it should
    take place, but isn't necessary.
    
    -- 
     Simon Riggs                   http://www.2ndQuadrant.com/
     PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

25. Re: Review: Extra Daemons / bgworker

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-12-03T15:43:21Z

    Markus Wanner wrote:
    > On 12/03/2012 03:28 PM, Alvaro Herrera wrote:
    
    > >> Just like av_launcher does it now: set a flag in shared memory and
    > >> signal the postmaster (PMSIGNAL_START_AUTOVAC_WORKER).
    > > 
    > > I'm not sure how this works.  What process is in charge of setting such
    > > a flag?
    > 
    > The only process that currently starts background workers ... ehm ...
    > autovacuum workers is the autovacuum launcher. It uses the above
    > Postmaster Signal in autovacuum.c:do_start_autovacuum_worker() to have
    > the postmaster launch bg/autovac workers on demand.
    
    Oh, I understand that.  My question was more about what mechanism are
    you envisioning for new bgworkers.  What process would be in charge of
    sending such signals?  Would it be a persistent bgworker which would
    decide to start up other bgworkers based on external conditions such as
    timing?  And how would postmaster know which bgworker to start -- would
    it use the bgworker's name to find it in the registered workers list?
    
    The trouble with the above rough sketch is how does the coordinating
    bgworker communicate with postmaster.  Autovacuum has a very, um,
    peculiar mechanism to make this work: avlauncher sends a signal (which
    has a hardcoded-in-core signal number) and postmaster knows to start a
    generic avworker; previously avlauncher has set things up in shared
    memory, and the generic avworker knows where to look to morph into the
    specific bgworker required.  So postmaster never really looks at shared
    memory other than the signal number (which is the only use of shmem in
    postmaster that's acceptable, because it's been carefully coded to be
    robust).  This doesn't work for generic modules because we don't have a
    hardcoded signal number; if two modules wanted to start up generic
    bgworkers, how would postmaster know which worker-main function to call?
    
    Now, maybe we can make this work in some robust fashion ("robust"
    meaning we don't put postmaster at risk, which is of utmost importance;
    and this in turn means don't trust anything in shared memory.)  I don't
    say it's impossible; only that it needs some more thought and careful
    design.
    
    As posted, the feature is already useful and it'd be good to have it
    committed soon so that others can experiment with whatever sample
    bgworkers they see fit.  That will give us more insight on other API
    refinements we might need.
    
    I'm going to disappear on paternity leave, most likely later this week,
    or early next week; I would like to commit this patch before that.  When
    I'm back we can discuss other improvements.  That will give us several
    weeks before the end of the 9.3 development period to get these in.  Of
    course, other committers are welcome to improve the code in my absence.
    
    -- 
    Álvaro Herrera                http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

26. Re: Review: Extra Daemons / bgworker

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-12-03T15:44:46Z

    Simon Riggs wrote:
    > On 3 December 2012 15:17, Markus Wanner <markus@bluegap.ch> wrote:
    
    > > The only process that currently starts background workers ... ehm ...
    > > autovacuum workers is the autovacuum launcher. It uses the above
    > > Postmaster Signal in autovacuum.c:do_start_autovacuum_worker() to have
    > > the postmaster launch bg/autovac workers on demand.
    > 
    > My understanding was that the patch keep autovac workers and
    > background workers separate at this point.
    
    That is correct.
    
    > Is there anything to be gained *now* from merging those two concepts?
    > I saw that as refactoring that can occur once we are happy it should
    > take place, but isn't necessary.
    
    IMO it's a net loss in robustness of the autovac implementation.
    
    -- 
    Álvaro Herrera                http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

27. Re: Review: Extra Daemons / bgworker

    Markus Wanner <markus@bluegap.ch> — 2012-12-03T15:51:22Z

    On 12/03/2012 04:27 PM, Simon Riggs wrote:
    > My understanding was that the patch keep autovac workers and
    > background workers separate at this point.
    
    I agree to that, for this patch. However, that might not keep me from
    trying to (re-)sumbit some of the bgworker patches in my queue. :-)
    
    Regards
    
    Markus Wanner
    
    
    

28. Re: Review: Extra Daemons / bgworker

    Markus Wanner <markus@bluegap.ch> — 2012-12-03T15:57:03Z

    On 12/03/2012 04:44 PM, Alvaro Herrera wrote:
    > Simon Riggs wrote:
    >> Is there anything to be gained *now* from merging those two concepts?
    >> I saw that as refactoring that can occur once we are happy it should
    >> take place, but isn't necessary.
    > 
    > IMO it's a net loss in robustness of the autovac implementation.
    
    Agreed.
    
    That's only one aspect of it, though. From the other point of view, it
    would be a proof of stability for the bgworker implementation if
    autovacuum worked on top of it.
    
    Regards
    
    Markus Wanner
    
    
    

29. Re: Review: Extra Daemons / bgworker

    Markus Wanner <markus@bluegap.ch> — 2012-12-03T16:23:42Z

    Alvaro,
    
    in general, please keep in mind that there are two aspects that I tend
    to mix and confuse: one is what's implemented and working for
    Postgres-R. The other this is how I envision (parts of it) to be merged
    back into Postgres, itself. Sorry if that causes confusion.
    
    On 12/03/2012 04:43 PM, Alvaro Herrera wrote:
    > Oh, I understand that.  My question was more about what mechanism are
    > you envisioning for new bgworkers.  What process would be in charge of
    > sending such signals?  Would it be a persistent bgworker which would
    > decide to start up other bgworkers based on external conditions such as
    > timing?  And how would postmaster know which bgworker to start -- would
    > it use the bgworker's name to find it in the registered workers list?
    
    Well, in the Postgres-R case, I've extended the autovacuum launcher to a
    more general purpose job scheduler, named coordinator. Lacking your
    bgworker patch, it is the only process that is able to launch background
    workers.
    
    Your work now allows extensions to register background workers. If I
    merge the two concepts, I can easily imagine allowing other (bgworker)
    processes to launch bgworkers.
    
    Another thing I can imagine is turning that coordinator into something
    that can schedule and trigger jobs registered by extensions (or even
    user defined ones). Think: cron daemon for SQL jobs in the background.
    (After all, that's pretty close to what the autovacuum launcher does,
    already.)
    
    > The trouble with the above rough sketch is how does the coordinating
    > bgworker communicate with postmaster.
    
    I know. I've gone through that pain.
    
    In Postgres-R, I've solved this with imessages (which was the primary
    reason for rejection of the bgworker patches back in 2010).
    
    The postmaster only needs to starts *a* background worker process. That
    process itself then has to figure out (by checking its imessage queue)
    what job it needs to perform. Or if you think in terms of bgworker
    types: what type of bgworker it needs to be.
    
    > Autovacuum has a very, um,
    > peculiar mechanism to make this work: avlauncher sends a signal (which
    > has a hardcoded-in-core signal number) and postmaster knows to start a
    > generic avworker; previously avlauncher has set things up in shared
    > memory, and the generic avworker knows where to look to morph into the
    > specific bgworker required.  So postmaster never really looks at shared
    > memory other than the signal number (which is the only use of shmem in
    > postmaster that's acceptable, because it's been carefully coded to be
    > robust).
    
    In Postgres-R, I've extended exactly that mechanism to work for other
    jobs that autovacuum.
    
    > This doesn't work for generic modules because we don't have a
    > hardcoded signal number; if two modules wanted to start up generic
    > bgworkers, how would postmaster know which worker-main function to call?
    
    You've just described above how this works for autovacuum: the
    postmaster doesn't *need* to know. Leave it to the bgworker to determine
    what kind of task it needs to perform.
    
    > As posted, the feature is already useful and it'd be good to have it
    > committed soon so that others can experiment with whatever sample
    > bgworkers they see fit.  That will give us more insight on other API
    > refinements we might need.
    
    I completely agree. I didn't ever intend to provide an alternative patch
    or hold you back. (Except for the extra daemon issue, where we disagree,
    but that's not a reason to keep this feature back). So please, go ahead
    and commit this feature (once the issues I've mentioned in the review
    are fixed).
    
    Please consider all of these plans or ideas in here (or in Postgres-R)
    as extending on your work, rather than competing against.
    
    > I'm going to disappear on paternity leave, most likely later this week,
    > or early next week; I would like to commit this patch before that.  When
    > I'm back we can discuss other improvements.  That will give us several
    > weeks before the end of the 9.3 development period to get these in.  Of
    > course, other committers are welcome to improve the code in my absence.
    
    Okay, thanks for sharing that. I'd certainly appreciate your inputs on
    further refinements for bgworkers and/or autovacuum.
    
    Regards
    
    Markus Wanner
    
    
    

30. Re: Review: Extra Daemons / bgworker

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-12-03T21:35:44Z

    So here's version 8.  This fixes a couple of bugs and most notably
    creates a separate PGPROC list for bgworkers, so that they don't
    interfere with client-connected backends.
    
    I tested starting 1000 non-shmem attached bgworkers -- postmaster
    doesn't break a sweat.  Also running 200 backend-connected bgworkers
    works fine (assuming they do nothing).  This is on my laptop, which is a
    dual-core Intel i5 processor.
    
    One notable thing is that I had to introduce this in the postmaster
    startup sequence:
    
        /*
         * process any libraries that should be preloaded at postmaster start
         */
        process_shared_preload_libraries();
    
        /*
         * If loadable modules have added background workers, MaxBackends needs to
         * be updated.  Do so now.
         */
        // RerunAssignHook("max_connections");
        if (GetNumShmemAttachedBgworkers() > 0)
            SetConfigOption("max_connections",
                            GetConfigOption("max_connections", false, false),
                            PGC_POSTMASTER, PGC_S_OVERRIDE);
    
    Note the intention here is to re-run the GUC assign hook for
    max_connections (hence the commented out hypothetical call to do so).
    Obviously, having to go through GetConfigOption and SetConfigOption is
    not a nice thing to do; we'll have to add some new entry point to guc.c
    for this to have a nicer interface.
    
    (I also observed that it's probably a good idea to have something like
    FunctionSetConfigOption for the places that are currently calling
    set_config_option directly).
    
    -- 
    Álvaro Herrera                http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
    

31. Re: Review: Extra Daemons / bgworker

    Markus Wanner <markus@bluegap.ch> — 2012-12-04T08:43:36Z

    On 12/03/2012 10:35 PM, Alvaro Herrera wrote:
    > So here's version 8.  This fixes a couple of bugs and most notably
    > creates a separate PGPROC list for bgworkers, so that they don't
    > interfere with client-connected backends.
    
    Nice, thanks. I'll try to review this again, shortly.
    
    Regards
    
    Markus Wanner
    
    
    

32. Re: Review: Extra Daemons / bgworker

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-12-04T15:05:30Z

    Alvaro Herrera wrote:
    
    > One notable thing is that I had to introduce this in the postmaster
    > startup sequence:
    > 
    >     /*
    >      * process any libraries that should be preloaded at postmaster start
    >      */
    >     process_shared_preload_libraries();
    > 
    >     /*
    >      * If loadable modules have added background workers, MaxBackends needs to
    >      * be updated.  Do so now.
    >      */
    >     // RerunAssignHook("max_connections");
    >     if (GetNumShmemAttachedBgworkers() > 0)
    >         SetConfigOption("max_connections",
    >                         GetConfigOption("max_connections", false, false),
    >                         PGC_POSTMASTER, PGC_S_OVERRIDE);
    > 
    > Note the intention here is to re-run the GUC assign hook for
    > max_connections (hence the commented out hypothetical call to do so).
    > Obviously, having to go through GetConfigOption and SetConfigOption is
    > not a nice thing to do; we'll have to add some new entry point to guc.c
    > for this to have a nicer interface.
    
    After fooling with guc.c to create such a new entry point, I decided
    that it looks too ugly.  guc.c is already complex enough with the API we
    have today that I don't want to be seen creating a partially-duplicate
    interface, even if it is going to result in simplified processing of
    this one place.  If we ever get around to needing another place to
    require rerunning a variable's assign_hook we can discuss it; for now it
    doesn't seem worth it.
    
    This is only called at postmaster start time, so it's not too
    performance-sensitive, hence SetConfigOption( .., GetConfigOption(), ..)
    seems acceptable from that POV.
    
    -- 
    Álvaro Herrera                http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

33. Re: Review: Extra Daemons / bgworker

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-12-05T15:09:58Z

    Here's a first attempt at a new documentation chapter.  This goes in
    part "Server Programming", just after the SPI chapter.
    
    I just noticed that worker_spi could use some more sample code, for
    example auth_counter was getting its own LWLock and also its own shmem
    area, which would be helpful to demonstrate I think.
    
    -- 
    Álvaro Herrera                http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
    

34. Re: Review: Extra Daemons / bgworker

    Andres Freund <andres@2ndquadrant.com> — 2012-12-05T21:29:01Z

    On 2012-12-05 12:09:58 -0300, Alvaro Herrera wrote:
    > Here's a first attempt at a new documentation chapter.  This goes in
    > part "Server Programming", just after the SPI chapter.
    >
    > I just noticed that worker_spi could use some more sample code, for
    > example auth_counter was getting its own LWLock and also its own shmem
    > area, which would be helpful to demonstrate I think.
    
    I am not exactly renowned for my english skills, but I have made a pass
    over the file made some slight changes and extended it in two places.
    I've also added a comment with a question that came to my mind when
    reading the docs...
    
    Greetings,
    
    Andres Freund
    
    --
     Andres Freund	                   http://www.2ndQuadrant.com/
     PostgreSQL Development, 24x7 Support, Training & Services
    

35. Re: Review: Extra Daemons / bgworker

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-12-05T21:42:42Z

    Andres Freund wrote:
    > On 2012-12-05 12:09:58 -0300, Alvaro Herrera wrote:
    > > Here's a first attempt at a new documentation chapter.  This goes in
    > > part "Server Programming", just after the SPI chapter.
    > >
    > > I just noticed that worker_spi could use some more sample code, for
    > > example auth_counter was getting its own LWLock and also its own shmem
    > > area, which would be helpful to demonstrate I think.
    > 
    > I am not exactly renowned for my english skills, but I have made a pass
    > over the file made some slight changes and extended it in two places.
    
    Thanks, I have applied it.
    
    > I've also added a comment with a question that came to my mind when
    > reading the docs...
    
    >    <para>
    >     <structfield>bgw_sighup</structfield> and <structfield>bgw_sigterm</> are
    >     pointers to functions that will be installed as signal handlers for the new
    > -   process.
    > +   process. XXX: Can they be NULL?
    >    </para>
    
    Hm.  The code doesn't check, so what happens is probably a bug anyhow.
    I don't know whether sigaction crashes in this case; its manpage doesn't
    say.  I guess the right thing to do is have RegisterBackgroundWorker
    check for a NULL sighandler, and set it to something standard if so (say
    SIG_IGN for SIGHUP and maybe quickdie() or similar for SIGTERM).
    
    -- 
    Álvaro Herrera                http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

36. Re: Review: Extra Daemons / bgworker

    Simon Riggs <simon@2ndquadrant.com> — 2012-12-05T21:46:03Z

    On 5 December 2012 15:09, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
    > Here's a first attempt at a new documentation chapter.  This goes in
    > part "Server Programming", just after the SPI chapter.
    >
    > I just noticed that worker_spi could use some more sample code, for
    > example auth_counter was getting its own LWLock and also its own shmem
    > area, which would be helpful to demonstrate I think.
    
    "to run once" -> "to run when"
    
    Prefer
    BgWorkerStart_ConsistentState to be renamed to BgWorkerRun_InHotStandby
    BgWorkerStart_RecoveryFinished to be renamed to BgWorkerRun_InNormalMode
    
    presumably a process will shutdown if (BgWorkerRun_InHotStandby &&
    !BgWorkerRun_InNormalMode)
    
    
    -- 
     Simon Riggs                   http://www.2ndQuadrant.com/
     PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

37. Re: Review: Extra Daemons / bgworker

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-12-05T22:03:44Z

    Simon Riggs wrote:
    > On 5 December 2012 15:09, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
    > > Here's a first attempt at a new documentation chapter.  This goes in
    > > part "Server Programming", just after the SPI chapter.
    > >
    > > I just noticed that worker_spi could use some more sample code, for
    > > example auth_counter was getting its own LWLock and also its own shmem
    > > area, which would be helpful to demonstrate I think.
    > 
    > "to run once" -> "to run when"
    
    Thanks.
    
    > Prefer
    > BgWorkerStart_ConsistentState to be renamed to BgWorkerRun_InHotStandby
    > BgWorkerStart_RecoveryFinished to be renamed to BgWorkerRun_InNormalMode
    > 
    > presumably a process will shutdown if (BgWorkerRun_InHotStandby &&
    > !BgWorkerRun_InNormalMode)
    
    Hmm, no, I haven't considered that.  You mean that a bgworker that
    specifies to start at BgWorkerStart_ConsistentState will stop once
    normal mode is reached?  Currently they don't do that.  And since we
    don't have the notion that workers stop working, it wouldn't work --
    postmaster would start them back up immediately.
    
    -- 
    Álvaro Herrera                http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

38. Re: Review: Extra Daemons / bgworker

    Andres Freund <andres@2ndquadrant.com> — 2012-12-05T22:22:54Z

    On 2012-12-05 19:03:44 -0300, Alvaro Herrera wrote:
    > Simon Riggs wrote:
    > > Prefer
    > > BgWorkerStart_ConsistentState to be renamed to BgWorkerRun_InHotStandby
    > > BgWorkerStart_RecoveryFinished to be renamed to BgWorkerRun_InNormalMode
    > >
    > > presumably a process will shutdown if (BgWorkerRun_InHotStandby &&
    > > !BgWorkerRun_InNormalMode)
    >
    > Hmm, no, I haven't considered that.  You mean that a bgworker that
    > specifies to start at BgWorkerStart_ConsistentState will stop once
    > normal mode is reached?  Currently they don't do that.  And since we
    > don't have the notion that workers stop working, it wouldn't work --
    > postmaster would start them back up immediately.
    
    I personally don't see too much demand for this from a use-case
    perspective. Simon, did you have anything in mind that made you ask
    this?
    
    Greetings,
    
    Andres Freund
    
    --
     Andres Freund	                   http://www.2ndQuadrant.com/
     PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

39. Re: Review: Extra Daemons / bgworker

    Andres Freund <andres@2ndquadrant.com> — 2012-12-05T22:25:02Z

    On 2012-12-05 18:42:42 -0300, Alvaro Herrera wrote:
    > >    <para>
    > >     <structfield>bgw_sighup</structfield> and <structfield>bgw_sigterm</> are
    > >     pointers to functions that will be installed as signal handlers for the new
    > > -   process.
    > > +   process. XXX: Can they be NULL?
    > >    </para>
    >
    > Hm.  The code doesn't check, so what happens is probably a bug anyhow.
    > I don't know whether sigaction crashes in this case; its manpage doesn't
    > say.  I guess the right thing to do is have RegisterBackgroundWorker
    > check for a NULL sighandler, and set it to something standard if so (say
    > SIG_IGN for SIGHUP and maybe quickdie() or similar for SIGTERM).
    
    Afair a NULL sigaction is used to query the current handler. Which
    indirectly might lead to problems due to the wrong handler being called.
    
    Setting up SIG_IGN and quickdie in that case seems to be sensible.
    
    Greetings,
    
    Andres Freund
    
    --
     Andres Freund	                   http://www.2ndQuadrant.com/
     PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

40. Re: Review: Extra Daemons / bgworker

    Simon Riggs <simon@2ndquadrant.com> — 2012-12-05T22:30:24Z

    On 5 December 2012 22:22, Andres Freund <andres@2ndquadrant.com> wrote:
    > On 2012-12-05 19:03:44 -0300, Alvaro Herrera wrote:
    >> Simon Riggs wrote:
    >> > Prefer
    >> > BgWorkerStart_ConsistentState to be renamed to BgWorkerRun_InHotStandby
    >> > BgWorkerStart_RecoveryFinished to be renamed to BgWorkerRun_InNormalMode
    >> >
    >> > presumably a process will shutdown if (BgWorkerRun_InHotStandby &&
    >> > !BgWorkerRun_InNormalMode)
    >>
    >> Hmm, no, I haven't considered that.  You mean that a bgworker that
    >> specifies to start at BgWorkerStart_ConsistentState will stop once
    >> normal mode is reached?  Currently they don't do that.  And since we
    >> don't have the notion that workers stop working, it wouldn't work --
    >> postmaster would start them back up immediately.
    >
    > I personally don't see too much demand for this from a use-case
    > perspective. Simon, did you have anything in mind that made you ask
    > this?
    
    Just clarifying how it worked, for the docs; happy with the way it is.
    
    -- 
     Simon Riggs                   http://www.2ndQuadrant.com/
     PostgreSQL Development, 24x7 Support, Training & Services
    
    
    

41. Re: Review: Extra Daemons / bgworker

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-12-06T04:59:31Z

    Andres Freund wrote:
    > On 2012-12-05 18:42:42 -0300, Alvaro Herrera wrote:
    > > >    <para>
    > > >     <structfield>bgw_sighup</structfield> and <structfield>bgw_sigterm</> are
    > > >     pointers to functions that will be installed as signal handlers for the new
    > > > -   process.
    > > > +   process. XXX: Can they be NULL?
    > > >    </para>
    > >
    > > Hm.  The code doesn't check, so what happens is probably a bug anyhow.
    > > I don't know whether sigaction crashes in this case; its manpage doesn't
    > > say.  I guess the right thing to do is have RegisterBackgroundWorker
    > > check for a NULL sighandler, and set it to something standard if so (say
    > > SIG_IGN for SIGHUP and maybe quickdie() or similar for SIGTERM).
    > 
    > Afair a NULL sigaction is used to query the current handler. Which
    > indirectly might lead to problems due to the wrong handler being called.
    > 
    > Setting up SIG_IGN and quickdie in that case seems to be sensible.
    
    Here's v9.  It adds that change, the tweaked docs, a bug fix (postmaster
    would kill itself if there's a process crash and a bgworker is stopped),
    and some pgindent and code reordering.
    github.com/alvherre/postgres/tree/bgworker contains this submission as
    commit fa4731c.
    
    I think we can get this committed as a useful starting point for this
    feature.
    
    -- 
    Álvaro Herrera                http://www.2ndQuadrant.com/
    PostgreSQL Development, 24x7 Support, Training & Services