Re: pgsql: Allow background workers to be started dynamically.

Allow background workers to be started dynamically.

There is a new API, RegisterDynamicBackgroundWorker, which allows
an ordinary user backend to register a new background writer during
normal running. This means that it's no longer necessary for all
background workers to be registered during processing of
shared_preload_libraries, although the option of registering workers
at that time remains available.

When a background worker exits and will not be restarted, the
slot previously used by that background worker is automatically
released and becomes available for reuse. Slots used by background
workers that are configured for automatic restart can't (yet) be
released without shutting down the system.

This commit adds a new source file, bgworker.c, and moves some
of the existing control logic for background workers there.
Previously, there was little enough logic that it made sense to
keep everything in postmaster.c, but not any more.

This commit also makes the worker_spi contrib module into an
extension and adds a new function, worker_spi_launch, which can
be used to demonstrate the new facility.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/7f7485a0cde92aa4ba235a1ffe4dda0ca0b6cc9a

Modified Files
--------------
contrib/worker_spi/Makefile | 3 +
contrib/worker_spi/worker_spi--1.0.sql | 9 +
contrib/worker_spi/worker_spi.c | 66 +++-
contrib/worker_spi/worker_spi.control | 5 +
doc/src/sgml/bgworker.sgml | 55 ++-
src/backend/postmaster/Makefile | 4 +-
src/backend/postmaster/bgworker.c | 483 +++++++++++++++++++++++++++
src/backend/postmaster/postmaster.c | 227 +++----------
src/backend/storage/ipc/ipci.c | 3 +
src/include/postmaster/bgworker.h | 14 +-
src/include/postmaster/bgworker_internals.h | 48 +++
src/include/storage/lwlock.h | 1 +
src/include/storage/pmsignal.h | 1 +
13 files changed, 710 insertions(+), 209 deletions(-)

Hi,

On 2013-07-16 17:02:47 +0000, Robert Haas wrote:
> Allow background workers to be started dynamically.
> ...
> src/include/postmaster/bgworker.h | 14 +-

The changes here make it impossible to write a bgworker which properly
works in 9.3 and 9.4. Was that intended? If so, the commit message
should mention the compatibility break...

That causes warnings like:
/home/andres/src/postgresql/contrib/bdr/bdr.c:655:24: warning: assignment from incompatible pointer type [enabled by default]
apply_worker.bgw_main = bdr_apply_main;
^
/home/andres/src/postgresql/contrib/bdr/bdr.c:744:25: error: incompatible types when assigning to type ‘char[64]’ from type ‘char *’
apply_worker.bgw_name = fullname;

And the bgw_name change will probably actually cause crashes...

If it was intended I propose changing the signature for 9.3 as
well. There's just no point in releasing 9.3 when we already know which
trivial but breaking change will be required for 9.4

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Jul 19, 2013 at 7:24 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> The changes here make it impossible to write a bgworker which properly
> works in 9.3 and 9.4. Was that intended? If so, the commit message
> should mention the compatibility break...

Yeah, sorry, I probably should have mentioned that. The structure
needs to be fixed size for us to store it in shared memory.

> If it was intended I propose changing the signature for 9.3 as
> well. There's just no point in releasing 9.3 when we already know which
> trivial but breaking change will be required for 9.4

I think that would be a good idea. And I'd also propose getting rid
of bgw_sighup and bgw_sigterm in both branches, while we're at it.
AFAICT, they don't add any functionality, and they're basically
unusable for dynamically started background workers. Probably better
not to get people to used to using them.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Jul 19, 2013 at 1:23 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Jul 19, 2013 at 7:24 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> The changes here make it impossible to write a bgworker which properly
>> works in 9.3 and 9.4. Was that intended? If so, the commit message
>> should mention the compatibility break...
>
> Yeah, sorry, I probably should have mentioned that. The structure
> needs to be fixed size for us to store it in shared memory.
>
>> If it was intended I propose changing the signature for 9.3 as
>> well. There's just no point in releasing 9.3 when we already know which
>> trivial but breaking change will be required for 9.4
>
> I think that would be a good idea. And I'd also propose getting rid
> of bgw_sighup and bgw_sigterm in both branches, while we're at it.
> AFAICT, they don't add any functionality, and they're basically
> unusable for dynamically started background workers. Probably better
> not to get people to used to using them.

+1. Much better to take that pain now, before we have made a production release.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Hi,

On 2013-07-19 08:23:25 -0400, Robert Haas wrote:
> On Fri, Jul 19, 2013 at 7:24 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > The changes here make it impossible to write a bgworker which properly
> > works in 9.3 and 9.4. Was that intended? If so, the commit message
> > should mention the compatibility break...
>
> Yeah, sorry, I probably should have mentioned that. The structure
> needs to be fixed size for us to store it in shared memory.

> > If it was intended I propose changing the signature for 9.3 as
> > well. There's just no point in releasing 9.3 when we already know which
> > trivial but breaking change will be required for 9.4
>
> I think that would be a good idea.

> And I'd also propose getting rid
> of bgw_sighup and bgw_sigterm in both branches, while we're at it.
> AFAICT, they don't add any functionality, and they're basically
> unusable for dynamically started background workers. Probably better
> not to get people to used to using them.

I don't have a problem with getting rid of those, it's easy enough to
register them inside the worker and it's safe since we start with
blocked signals. I seem to remember some discussion about why they were
added but I can't find a reference anymore. Alvaro, do you remember?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Andres Freund escribió:

> On 2013-07-19 08:23:25 -0400, Robert Haas wrote:

> > And I'd also propose getting rid
> > of bgw_sighup and bgw_sigterm in both branches, while we're at it.
> > AFAICT, they don't add any functionality, and they're basically
> > unusable for dynamically started background workers. Probably better
> > not to get people to used to using them.
>
> I don't have a problem with getting rid of those, it's easy enough to
> register them inside the worker and it's safe since we start with
> blocked signals. I seem to remember some discussion about why they were
> added but I can't find a reference anymore. Alvaro, do you remember?

I left them there because it was easy; but they were absolutely
necessary only until we decided that we would start the worker's main
function with signals blocked. I don't think there's any serious reason
not to remove them now.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sat, Jul 20, 2013 at 12:39 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
>> I don't have a problem with getting rid of those, it's easy enough to
>> register them inside the worker and it's safe since we start with
>> blocked signals. I seem to remember some discussion about why they were
>> added but I can't find a reference anymore. Alvaro, do you remember?
>
> I left them there because it was easy; but they were absolutely
> necessary only until we decided that we would start the worker's main
> function with signals blocked. I don't think there's any serious reason
> not to remove them now.

All right, done. FWIW, I think starting the worker's main with
signals blocked was definitely the right decision.

I think we have consensus to back-patch the other API changes as well.
I'll work up a patch for that.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Jul 22, 2013 at 3:16 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I think we have consensus to back-patch the other API changes as well.
> I'll work up a patch for that.

Pushed that as well.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company