On Fri, Oct 11, 2013 at 9:27 AM, Michael Paquier <michael.paqu...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers