On Fri, Oct 11, 2013 at 9:27 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Few comments about the code:
> 1) In postmaster.c, what about adding a comment here telling that we
> can forget about this bgworker as it has already been requested for a
> termination:
> + if (rw->rw_worker.bgw_restart_time == BGW_NEVER_RESTART
> + || rw->rw_terminate)
I think that'd be just repeating what the code already says.
> 2) Trying to think about this patch as an independent feature, I think
> that it would have been nice to demonstrate the capacity of
> TerminateBackgroundWorker directly with an example in worker_spi to
> show a direct application of it, with for example the addition of a
> function like worker_spi_terminate. However this needs:
> - either an additional API using as input the PID, pid used to fetch a
> bgworker handle terminating the worker afterwards. This is basically
> what I did in the patch attached when playing with your patch. You
> might find it useful... Or not! It introduces as well
I think this is kind of missing the point of this interface. If
you've already got the PID, you're can just kill the PID yourself.
But suppose you've just registered a background worker in the parent
process, and then there's an ERROR. You want the background worker to
die, since you don't need it any more, but the postmaster might not
have even started it yet, so there's no PID. That's the case in which
this is really useful. It's not a good match for what worker_spi
needs, because in the worker_spi, you're intending to start a process
that you *expect* to outlive the user backend's transaction, and even
the user backend's lifetime.
I agree we need better testing for this code, but the problem is that
this is all pretty low-level plumbing. This patch set was prompted by
problems that came up when I tried to build an application using this
facility; and I don't know that this is the last patch that will be
needed to solve all of those problems. I'm working on another patch
set that adds a shared-memory message queue through which messages can
be pushed, and I think that will lend itself well to testing of both
background workers and dynamic shared memory. I hope to have that
ready in time for the next CommitFest.
> 3) Documentation is needed, following comment 2).
Good point.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
