Re: dynamic background workers

Parallel query, or any subset of that project such as parallel sort,
will require a way to start background workers on demand. Thanks to
Alvaro's work on 9.3, we now have the ability to configure background
workers via shared_preload_libraries. But if you don't have the right
library loaded at startup time, and subsequently wish to add a
background worker while the server is running, you are out of luck.
Even if you do have the right library loaded, but want to start
workers in response to user activity, rather than when the database
comes on-line, you are also out of luck. Relaxing these restrictions
is essential for parallel query (or parallel processing of any kind),
and useful apart from that. Two patches are attached.

The first patch, max-worker-processes-v1.patch, adds a new GUC
max_worker_processes, which defaults to 8. This fixes the problem
discussed here:

http://www.postgresql.org/message-id/CA+TgmobguVO+qHnHvxBA2TFkDhw67Y=4Bp405FVABEc_EtO4VQ@mail.gmail.com

Apart from fixing that problem, it's a pretty boring patch.

The second patch, dynamic-bgworkers-v1.patch, revises the background
worker API to allow background workers to be started dynamically.
This requires some communication channel from ordinary workers to the
postmaster, because it is the postmaster that must ultimately start
the newly-registered workers. However, that communication channel has
to be designed pretty carefully, lest a shared memory corruption take
out the postmaster and lead to inadvertent failure to restart after a
crash. Here's how I implemented that: there's an array in shared
memory of a size equal to max_worker_processes. This array is
separate from the backend-private list of workers maintained by the
postmaster, but the two are kept in sync. When a new background
worker registration is added to the shared data structure, the backend
adding it uses the existing pmsignal mechanism to kick the postmaster,
which then scans the array for new registrations. I have attempted to
make the code that transfers the shared_memory state into the
postmaster's private state as paranoid as humanly possible. The
precautions taken are documented in the comments. Conversely, when a
background worker flagged as BGW_NEVER_RESTART is considered for
restart (and we decide against it), the corresponding slot in the
shared memory array is marked as no longer in use, allowing it to be
reused for a new registration.

Since the postmaster cannot take locks, synchronization between the
postmaster and other backends using the shared memory segment has to
be lockless. This mechanism is also documented in the comments. An
lwlock is used to prevent two backends that are both registering a new
worker at about the same time from stomping on each other, but the
postmaster need not care about that lwlock.

This patch also extends worker_spi as a demonstration of the new
interface. With this patch, you can CREATE EXTENSION worker_spi and
then call worker_spi_launch(int4) to launch a new background worker,
or combine it with generate_series() to launch a bunch at once. Then
you can kill them off with pg_terminate_backend() and start some new
ones. That, in my humble opinion, is pretty cool.

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

On Sat, Jun 15, 2013 at 6:00 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> The second patch, dynamic-bgworkers-v1.patch, revises the background
> worker API to allow background workers to be started dynamically.
> This requires some communication channel from ordinary workers to the
> postmaster, because it is the postmaster that must ultimately start
> the newly-registered workers. However, that communication channel has
> to be designed pretty carefully, lest a shared memory corruption take
> out the postmaster and lead to inadvertent failure to restart after a
> crash. Here's how I implemented that: there's an array in shared
> memory of a size equal to max_worker_processes. This array is
> separate from the backend-private list of workers maintained by the
> postmaster, but the two are kept in sync. When a new background
> worker registration is added to the shared data structure, the backend
> adding it uses the existing pmsignal mechanism to kick the postmaster,
> which then scans the array for new registrations. I have attempted to
> make the code that transfers the shared_memory state into the
> postmaster's private state as paranoid as humanly possible. The
> precautions taken are documented in the comments. Conversely, when a
> background worker flagged as BGW_NEVER_RESTART is considered for
> restart (and we decide against it), the corresponding slot in the
> shared memory array is marked as no longer in use, allowing it to be
> reused for a new registration.
>
> Since the postmaster cannot take locks, synchronization between the
> postmaster and other backends using the shared memory segment has to
> be lockless. This mechanism is also documented in the comments. An
> lwlock is used to prevent two backends that are both registering a new
> worker at about the same time from stomping on each other, but the
> postmaster need not care about that lwlock.
>
> This patch also extends worker_spi as a demonstration of the new
> interface. With this patch, you can CREATE EXTENSION worker_spi and
> then call worker_spi_launch(int4) to launch a new background worker,
> or combine it with generate_series() to launch a bunch at once. Then
> you can kill them off with pg_terminate_backend() and start some new
> ones. That, in my humble opinion, is pretty cool.
>
This looks really interesting, +1. I'll test the patch if possible next
week.
--
Michael

On 14 June 2013 22:00, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Parallel query, or any subset of that project such as parallel sort,
> will require a way to start background workers on demand. Thanks to
> Alvaro's work on 9.3, we now have the ability to configure background
> workers via shared_preload_libraries. But if you don't have the right
> library loaded at startup time, and subsequently wish to add a
> background worker while the server is running, you are out of luck.
> Even if you do have the right library loaded, but want to start
> workers in response to user activity, rather than when the database
> comes on-line, you are also out of luck. Relaxing these restrictions
> is essential for parallel query (or parallel processing of any kind),
> and useful apart from that. Two patches are attached.

Your proposal is exactly what we envisaged and parallel query always
was a target for background workers. The restrictions were only there
to ensure we got the feature into 9.3, rather than trying to implement
everything and then having it pushed back a release.

So +1.

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

BTW, one of the ideas that popped up in the unConference session on
replication was "why couldn't we use a background worker as a replication
agent?"

The main reason pointed out was 'because that means you have to restart the
postmaster to add a replication agent.' (e.g. - like a Slony "slon"
process)

There may well be other better reasons not to do so, but it would be nice
to eliminate this reason. It seems seriously limiting to the bg-worker
concept for them to be thus restricted.
--
When confronted by a difficult problem, solve it by reducing it to the
question, "How would the Lone Ranger handle this?"

On Fri, 2013-06-14 at 17:00 -0400, Robert Haas wrote:
> Alvaro's work on 9.3, we now have the ability to configure background
> workers via shared_preload_libraries. But if you don't have the right
> library loaded at startup time, and subsequently wish to add a
> background worker while the server is running, you are out of luck.

We could tweak shared_preload_libraries so that it reacts sensibly to
reloads. I basically gave up on that by writing
session_preload_libraries, but if there is more general use for that, we
could try.

(That doesn't invalidate your work, but it's a thought.)

Hi,

On Sat, Jun 15, 2013 at 6:00 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> The first patch, max-worker-processes-v1.patch, adds a new GUC
> max_worker_processes, which defaults to 8. This fixes the problem
> discussed here:
>
> http://www.postgresql.org/message-id/CA+TgmobguVO+qHnHvxBA2TFkDhw67Y=4Bp405FVABEc_EtO4VQ@mail.gmail.com
>
> Apart from fixing that problem, it's a pretty boring patch.
I just had a look at the first patch which is pretty simple before
looking in details at the 2nd patch.

Here are some minor comments:
1) Correction of postgresql.conf.sample
Putting the new parameter in the section resource usage is adapted,
however why not adding a new sub-section of this type with some
comments like below?
# - Background workers -
#max_worker_processes = 8 # Maximum number of background worker subprocesses
# (change requires restart)
2) Perhaps it would be better to specify in the docs that if the
number of bgworker that are tried to be started by server is higher
than max_worker_processes, startup of the extra bgworkers will fail
but server will continue running as if nothing happened. This is
something users should be made aware of.
3) In InitProcGlobal:proc.c, wouldn't it be more consistent to do that
when assigning new slots in PGPROC:
else if (i < MaxConnections + autovacuum_max_workers + max_worker_processes + 1)
{
/* PGPROC for bgworker, add to bgworkerFreeProcs list */
procs[i].links.next = (SHM_QUEUE *) ProcGlobal->bgworkerFreeProcs;
ProcGlobal->bgworkerFreeProcs = &procs[i];
}
instead of that?
else if (i < MaxBackends)
{
/* PGPROC for bgworker, add to bgworkerFreeProcs list */
procs[i].links.next = (SHM_QUEUE *) ProcGlobal->bgworkerFreeProcs;
ProcGlobal->bgworkerFreeProcs = &procs[i];
}

I have also done many tests with worker_spi and some home-made
bgworkers and the patch is working as expected, the extra bgworkers
are not started once the maximum number set it reached.
I'll try to look at the other patch soon, but I think that the real
discussion on the topic is just beginning... Btw, IMHO, this first
patch can safely be committed as we would have a nice base for future
discussions/reviews.
Regards,
--
Michael

On Mon, Jun 17, 2013 at 10:45 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On Fri, 2013-06-14 at 17:00 -0400, Robert Haas wrote:
>> Alvaro's work on 9.3, we now have the ability to configure background
>> workers via shared_preload_libraries. But if you don't have the right
>> library loaded at startup time, and subsequently wish to add a
>> background worker while the server is running, you are out of luck.
>
> We could tweak shared_preload_libraries so that it reacts sensibly to
> reloads. I basically gave up on that by writing
> session_preload_libraries, but if there is more general use for that, we
> could try.
>
> (That doesn't invalidate your work, but it's a thought.)

Yeah, I thought about that. But it doesn't seem possible to do
anything all that sane. You can't unload libraries if they've been
removed; you can potentially load new ones if they've been added. But
that's a bit confusing, if the config file says that's what's loaded
is bar, and what's actually loaded is foo, bar, baz, bletch, and quux.

Some variant of this might still be worth doing, but figuring out the
details sounded like more than I wanted to get into, so I punted. :-)

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

Robert,

On 06/14/2013 11:00 PM, Robert Haas wrote:
> Parallel query, or any subset of that project such as parallel sort,
> will require a way to start background workers on demand.

thanks for continuing this, very much appreciated. Postgres-R and thus
TransLattice successfully use a similar approach for years, now.

I only had a quick glance over the patch, yet. Some comments on the design:

> This requires some communication channel from ordinary workers to the
> postmaster, because it is the postmaster that must ultimately start
> the newly-registered workers. However, that communication channel has
> to be designed pretty carefully, lest a shared memory corruption take
> out the postmaster and lead to inadvertent failure to restart after a
> crash. Here's how I implemented that: there's an array in shared
> memory of a size equal to max_worker_processes. This array is
> separate from the backend-private list of workers maintained by the
> postmaster, but the two are kept in sync. When a new background
> worker registration is added to the shared data structure, the backend
> adding it uses the existing pmsignal mechanism to kick the postmaster,
> which then scans the array for new registrations.

That sounds like a good simplification. Even if it's an O(n) operation,
the n in question here has relatively low practical limits. It's
unlikely to be much of a concern, I guess.

Back then, I solved it by having a "fork request slot". After starting,
the bgworker then had to clear that slot and register with a coordinator
process (i.e. the original requestor), so that one learned its fork
request was successful. At some point I expanded that to multiple
request slots to better handle multiple concurrent fork requests.
However, it was difficult to get right and requires more IPC than your
approach.

On the pro side: The shared memory area used by the postmaster was very
small in size and read-only to the postmaster. These were my main goals,
which I'm not sure were the best ones, now that I read your concept.

> I have attempted to
> make the code that transfers the shared_memory state into the
> postmaster's private state as paranoid as humanly possible. The
> precautions taken are documented in the comments. Conversely, when a
> background worker flagged as BGW_NEVER_RESTART is considered for
> restart (and we decide against it), the corresponding slot in the
> shared memory array is marked as no longer in use, allowing it to be
> reused for a new registration.

Sounds like the postmaster is writing to shared memory. Not sure why
I've been trying so hard to avoid that, though. After all, it can hardly
hurt itself *writing* to shared memory.

> Since the postmaster cannot take locks, synchronization between the
> postmaster and other backends using the shared memory segment has to
> be lockless. This mechanism is also documented in the comments. An
> lwlock is used to prevent two backends that are both registering a new
> worker at about the same time from stomping on each other, but the
> postmaster need not care about that lwlock.
>
> This patch also extends worker_spi as a demonstration of the new
> interface. With this patch, you can CREATE EXTENSION worker_spi and
> then call worker_spi_launch(int4) to launch a new background worker,
> or combine it with generate_series() to launch a bunch at once. Then
> you can kill them off with pg_terminate_backend() and start some new
> ones. That, in my humble opinion, is pretty cool.

It definitely is. Thanks again.

Regards

Markus Wanner

On Thu, Jun 20, 2013 at 9:57 AM, Markus Wanner <markus(at)bluegap(dot)ch> wrote:
> That sounds like a good simplification. Even if it's an O(n) operation,
> the n in question here has relatively low practical limits. It's
> unlikely to be much of a concern, I guess.

The constant factor is also very small. Generally, I would expect
num_worker_processes <~ # CPUs, and scanning a 32, 64, or even 128
element array is not a terribly time-consuming operation. We might
need to re-think this when systems with 4096 processors become
commonplace, but considering how many other things would also need to
be fixed to work well in that universe, I'm not too concerned about it
just yet.

One thing I think we probably want to explore in the future, for both
worker backends and regular backends, is pre-forking. We could avoid
some of the latency associated with starting up a new backend or
opening a new connection in that way. However, there are quite a few
details to be thought through there, so I'm not eager to pursue that
just yet. Once we have enough infrastructure to implement meaningful
parallelism, we can benchmark it and find out where the bottlenecks
are, and which solutions actually help most.

> Back then, I solved it by having a "fork request slot". After starting,
> the bgworker then had to clear that slot and register with a coordinator
> process (i.e. the original requestor), so that one learned its fork
> request was successful. At some point I expanded that to multiple
> request slots to better handle multiple concurrent fork requests.
> However, it was difficult to get right and requires more IPC than your
> approach.

I do think we need a mechanism to allow the backend that requested the
bgworker to know whether or not the bgworker got started, and whether
it unexpectedly died. Once we get to the point of calling user code
within the bgworker process, it can use any number of existing
mechanisms to make sure that it won't die without notifying the
backend that started it (short of a PANIC, in which case it won't
matter anyway). But we need a way to report failures that happen
before that point. I have some ideas about that, but decided to leave
them for future passes. The remit of this patch is just to make it
possible to dynamically register bgworkers. Allowing a bgworker to be
"tied" to the session that requested it via some sort of feedback loop
is a separate project - which I intend to tackle before CF2, assuming
this gets committed (and so far nobody is objecting to that).

>> I have attempted to
>> make the code that transfers the shared_memory state into the
>> postmaster's private state as paranoid as humanly possible. The
>> precautions taken are documented in the comments. Conversely, when a
>> background worker flagged as BGW_NEVER_RESTART is considered for
>> restart (and we decide against it), the corresponding slot in the
>> shared memory array is marked as no longer in use, allowing it to be
>> reused for a new registration.
>
> Sounds like the postmaster is writing to shared memory. Not sure why
> I've been trying so hard to avoid that, though. After all, it can hardly
> hurt itself *writing* to shared memory.

I think there's ample room for paranoia about postmaster interaction
with shared memory, but all it's doing is setting a flag, which is no
different from what CheckPostmasterSignal() already does.

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

On 06/20/2013 04:41 PM, Robert Haas wrote:
> The constant factor is also very small. Generally, I would expect
> num_worker_processes <~ # CPUs

That assumption might hold for parallel querying, yes. In case of
Postgres-R, it doesn't. In the worst case, i.e. with a 100% write load,
a cluster of n nodes, each with m backends performing transactions, all
of them replicated to all other (n-1) nodes, you end up with ((n-1) * m)
bgworkers. Which is pretty likely to be way above the # CPUs on any
single node.

I can imagine other extensions or integral features like autonomous
transactions that might possibly want many more bgworkers as well.

> and scanning a 32, 64, or even 128
> element array is not a terribly time-consuming operation.

I'd extend that to say scanning an array with a few thousand elements is
not terribly time-consuming, either. IMO the simplicity is worth it,
ATM. It's all relative to your definition of ... eh ... "terribly".

.oO( ... premature optimization ... all evil ... )

> We might
> need to re-think this when systems with 4096 processors become
> commonplace, but considering how many other things would also need to
> be fixed to work well in that universe, I'm not too concerned about it
> just yet.

Agreed.

> One thing I think we probably want to explore in the future, for both
> worker backends and regular backends, is pre-forking. We could avoid
> some of the latency associated with starting up a new backend or
> opening a new connection in that way. However, there are quite a few
> details to be thought through there, so I'm not eager to pursue that
> just yet. Once we have enough infrastructure to implement meaningful
> parallelism, we can benchmark it and find out where the bottlenecks
> are, and which solutions actually help most.

Do you mean pre-forking and connecting to a specific database? Or really
just the forking?

> I do think we need a mechanism to allow the backend that requested the
> bgworker to know whether or not the bgworker got started, and whether
> it unexpectedly died. Once we get to the point of calling user code
> within the bgworker process, it can use any number of existing
> mechanisms to make sure that it won't die without notifying the
> backend that started it (short of a PANIC, in which case it won't
> matter anyway). But we need a way to report failures that happen
> before that point. I have some ideas about that, but decided to leave
> them for future passes. The remit of this patch is just to make it
> possible to dynamically register bgworkers. Allowing a bgworker to be
> "tied" to the session that requested it via some sort of feedback loop
> is a separate project - which I intend to tackle before CF2, assuming
> this gets committed (and so far nobody is objecting to that).

Okay, sounds good. Given my background, I considered that a solved
problem. Thanks for pointing it out.

>> Sounds like the postmaster is writing to shared memory. Not sure why
>> I've been trying so hard to avoid that, though. After all, it can hardly
>> hurt itself *writing* to shared memory.
>
> I think there's ample room for paranoia about postmaster interaction
> with shared memory, but all it's doing is setting a flag, which is no
> different from what CheckPostmasterSignal() already does.

Sounds good to me.

Regards

Markus Wanner

On Thu, Jun 20, 2013 at 10:59 AM, Markus Wanner <markus(at)bluegap(dot)ch> wrote:
> On 06/20/2013 04:41 PM, Robert Haas wrote:
>> The constant factor is also very small. Generally, I would expect
>> num_worker_processes <~ # CPUs
>
> That assumption might hold for parallel querying, yes. In case of
> Postgres-R, it doesn't. In the worst case, i.e. with a 100% write load,
> a cluster of n nodes, each with m backends performing transactions, all
> of them replicated to all other (n-1) nodes, you end up with ((n-1) * m)
> bgworkers. Which is pretty likely to be way above the # CPUs on any
> single node.
>
> I can imagine other extensions or integral features like autonomous
> transactions that might possibly want many more bgworkers as well.

Yeah, maybe. I think in general it's not going to work great to have
zillions of backends floating around, because eventually the OS
scheduler overhead - and the memory overhead - are going to become
pain points. And I'm hopeful that autonomous transactions can be
implemented without needing to start a new backend for each one,
because that sounds pretty expensive. Some users of other database
products will expect autonomous transactions to be cheap; aside from
that, cheap is better than expensive. But we will see. At any rate I
think your basic point is that people might end up creating a lot more
background workers than I'm imagining, which is certainly a fair
point.

>> and scanning a 32, 64, or even 128
>> element array is not a terribly time-consuming operation.
>
> I'd extend that to say scanning an array with a few thousand elements is
> not terribly time-consuming, either. IMO the simplicity is worth it,
> ATM. It's all relative to your definition of ... eh ... "terribly".
>
> .oO( ... premature optimization ... all evil ... )

Yeah, that thing.

>> One thing I think we probably want to explore in the future, for both
>> worker backends and regular backends, is pre-forking. We could avoid
>> some of the latency associated with starting up a new backend or
>> opening a new connection in that way. However, there are quite a few
>> details to be thought through there, so I'm not eager to pursue that
>> just yet. Once we have enough infrastructure to implement meaningful
>> parallelism, we can benchmark it and find out where the bottlenecks
>> are, and which solutions actually help most.
>
> Do you mean pre-forking and connecting to a specific database? Or really
> just the forking?

I've considered both at various times, although in this context I was
mostly thinking about just the forking. Pre-connecting to a specific
database would save an unknown but possibly significant amount of
additional latency. Against that, it's more complex (because we've
got to track which preforked workers are associated with which
databases) and there's some cost to guessing wrong (because then we're
keeping workers around that we can't use, or maybe even having to turn
around and kill them to make slots for the workers we actually need).
I suspect we'll want to pursue the idea at some point but it's not
near the top of my list.

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

On 2013-06-20 11:29:27 -0400, Robert Haas wrote:
> > Do you mean pre-forking and connecting to a specific database? Or really
> > just the forking?
>
> I've considered both at various times, although in this context I was
> mostly thinking about just the forking. Pre-connecting to a specific
> database would save an unknown but possibly significant amount of
> additional latency. Against that, it's more complex (because we've
> got to track which preforked workers are associated with which
> databases) and there's some cost to guessing wrong (because then we're
> keeping workers around that we can't use, or maybe even having to turn
> around and kill them to make slots for the workers we actually need).
> I suspect we'll want to pursue the idea at some point but it's not
> near the top of my list.

Just as a datapoint, if you benchmark the numbers of forks that can be
performed by a single process (i.e. postmaster) the number is easily in
the 10s of thousands. Now forking that much has some scalability
implications inside the kernel, but still.
I'd be surprised if the actual fork is more than 5-10% of the current
cost of starting a new backend.

Greetings,

Andres Freund

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

Hi,

Please find some review for the 2nd patch, with the 1st patch applied
on top of it.

On Sat, Jun 15, 2013 at 6:00 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> The second patch, dynamic-bgworkers-v1.patch, revises the background
> worker API to allow background workers to be started dynamically.
> This requires some communication channel from ordinary workers to the
> postmaster, because it is the postmaster that must ultimately start
> the newly-registered workers. However, that communication channel has
> to be designed pretty carefully, lest a shared memory corruption take
> out the postmaster and lead to inadvertent failure to restart after a
> crash. Here's how I implemented that: there's an array in shared
> memory of a size equal to max_worker_processes. This array is
> separate from the backend-private list of workers maintained by the
> postmaster, but the two are kept in sync. When a new background
> worker registration is added to the shared data structure, the backend
> adding it uses the existing pmsignal mechanism to kick the postmaster,
> which then scans the array for new registrations. I have attempted to
> make the code that transfers the shared_memory state into the
> postmaster's private state as paranoid as humanly possible. The
> precautions taken are documented in the comments. Conversely, when a
> background worker flagged as BGW_NEVER_RESTART is considered for
> restart (and we decide against it), the corresponding slot in the
> shared memory array is marked as no longer in use, allowing it to be
> reused for a new registration.
>
> Since the postmaster cannot take locks, synchronization between the
> postmaster and other backends using the shared memory segment has to
> be lockless. This mechanism is also documented in the comments. An
> lwlock is used to prevent two backends that are both registering a new
> worker at about the same time from stomping on each other, but the
> postmaster need not care about that lwlock.
>
> This patch also extends worker_spi as a demonstration of the new
> interface. With this patch, you can CREATE EXTENSION worker_spi and
> then call worker_spi_launch(int4) to launch a new background worker,
> or combine it with generate_series() to launch a bunch at once. Then
> you can kill them off with pg_terminate_backend() and start some new
> ones. That, in my humble opinion, is pretty cool.

The patch applies cleanly, I found a couple of whitespaces though:
/home/ioltas/download/dynamic-bgworkers-v1.patch:452: space before tab
in indent.
slot = &BackgroundWorkerData->slot[rw->rw_shmem_slot];
/home/ioltas/download/dynamic-bgworkers-v1.patch:474: space before tab
in indent.
slot = &BackgroundWorkerData->slot[slotno];
/home/ioltas/download/dynamic-bgworkers-v1.patch:639: trailing whitespace.
success = true;
warning: 3 lines add whitespace errors.

The code compiles, has no warnings, and make check passes.

Then, here are some impressions after reading the code. It is good to
see that all the bgworker APIs are moved under the same banner
bgworker.c.
1) Just for clarity, I think that this code in worker_spi.c deserves a
comment mentioning that this code path cannot cannot be taken for a
bgworker not loaded via shared_preload_libraries.
+
+ if (!process_shared_preload_libraries_in_progress)
+ return;
+
2) s/NUL-terminated/NULL-terminated @ bgworker.c
3) Why not adding an other function in worker_spi.c being the opposite
of worker_spi_launch to stop dynamic bgworkers for a given index
number? This would be only a wrapper of pg_terminate_backend, OK, but
at least it would give the user all the information needed to start
and to stop a dynamic bgworker with a single extension, here
worker_spi.c. It can be painful to stop
4) Not completely related to this patch, but one sanity check in
SanityCheckBackgroundWorker:bgworker.c is not listed in the
documentation: when requesting a database connection, bgworker needs
to have access to shmem. It looks that this should be also fixed in
REL9_3_STABLE.
5) Why not adding some documentation? Both dynamic and static
bgworkers share the same infrastructure, so some lines in the existing
chapter might be fine?
6) Just wondering something: it looks that the current code is not
able to identify what was the way used to start a given bgworker.
Would it be interesting to be able to identify if a bgworker has been
registered though RegisterBackgroundWorker or
RegisterDynamicBackgroundWorker?

I have also done some tests, and the infrastructure is working nicely.
The workers started dynamically are able to receive SIGHUP and
SIGTERM. Workers are also not started if the maximum number of
bgworkers authorized is reached. It is really a nice feature!

Also, I will try to do some more tests to test the robustness of the
slist and the protocol used.

Regards,
--
Michael

On Mon, Jun 24, 2013 at 3:51 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> 3) Why not adding an other function in worker_spi.c being the opposite
> of worker_spi_launch to stop dynamic bgworkers for a given index
> number? This would be only a wrapper of pg_terminate_backend, OK, but
> at least it would give the user all the information needed to start
> and to stop a dynamic bgworker with a single extension, here
> worker_spi.c. It can be painful to stop

Well, there's currently no mechanism for the person who starts a new
backend to know the PID of the process that actually got started. I
plan to write a patch to address that problem, but it's not this
patch.

> 4) Not completely related to this patch, but one sanity check in
> SanityCheckBackgroundWorker:bgworker.c is not listed in the
> documentation: when requesting a database connection, bgworker needs
> to have access to shmem. It looks that this should be also fixed in
> REL9_3_STABLE.

That's fine; I think it's separate from this patch. Please feel free
to propose something.

> 5) Why not adding some documentation? Both dynamic and static
> bgworkers share the same infrastructure, so some lines in the existing
> chapter might be fine?

I'll take a look.

> 6) Just wondering something: it looks that the current code is not
> able to identify what was the way used to start a given bgworker.
> Would it be interesting to be able to identify if a bgworker has been
> registered though RegisterBackgroundWorker or
> RegisterDynamicBackgroundWorker?

I don't think that's a good thing to expose.

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

Andres Freund escribió:

> Just as a datapoint, if you benchmark the numbers of forks that can be
> performed by a single process (i.e. postmaster) the number is easily in
> the 10s of thousands. Now forking that much has some scalability
> implications inside the kernel, but still.
> I'd be surprised if the actual fork is more than 5-10% of the current
> cost of starting a new backend.

I played at having some thousands of registered bgworkers on my laptop,
and there wasn't even that much load. So yeah, you can have lots of
forks.

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

On Wed, Jul 3, 2013 at 11:19 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Jun 24, 2013 at 3:51 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> 3) Why not adding an other function in worker_spi.c being the opposite
>> of worker_spi_launch to stop dynamic bgworkers for a given index
>> number? This would be only a wrapper of pg_terminate_backend, OK, but
>> at least it would give the user all the information needed to start
>> and to stop a dynamic bgworker with a single extension, here
>> worker_spi.c. It can be painful to stop
>
> Well, there's currently no mechanism for the person who starts a new
> backend to know the PID of the process that actually got started. I
> plan to write a patch to address that problem, but it's not this
> patch.
OK. Understood, this functionality would be a good addition to have.

>> 4) Not completely related to this patch, but one sanity check in
>> SanityCheckBackgroundWorker:bgworker.c is not listed in the
>> documentation: when requesting a database connection, bgworker needs
>> to have access to shmem. It looks that this should be also fixed in
>> REL9_3_STABLE.
>
> That's fine; I think it's separate from this patch. Please feel free
> to propose something.
I'll send a patch about that.

>> 6) Just wondering something: it looks that the current code is not
>> able to identify what was the way used to start a given bgworker.
>> Would it be interesting to be able to identify if a bgworker has been
>> registered though RegisterBackgroundWorker or
>> RegisterDynamicBackgroundWorker?
>
> I don't think that's a good thing to expose.
My concerns here are covered by the functionality you propose in 1),
where a user who launched a custom bgworker would know its PID, this
would allow users to keep track of which bgworker has been started
dynamically.

Regards,
--
Michael

On Wed, Jul 3, 2013 at 11:15 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Andres Freund escribió:
>> Just as a datapoint, if you benchmark the numbers of forks that can be
>> performed by a single process (i.e. postmaster) the number is easily in
>> the 10s of thousands. Now forking that much has some scalability
>> implications inside the kernel, but still.
>> I'd be surprised if the actual fork is more than 5-10% of the current
>> cost of starting a new backend.
>
> I played at having some thousands of registered bgworkers on my laptop,
> and there wasn't even that much load. So yeah, you can have lots of
> forks.

Since no one seems to be objecting to this patch beyond the lack of
documentation, I've added documentation and committed it, with
appropriate rebasing and a few minor cleanups. One loose end is
around the bgw_sighup and bgw_sigterm structure members. If you're
registering a background worker for a library that is not loaded in
the postmaster, you can't (safely) use these for anything, because
it's possible (though maybe not likely) for the worker process to map
the shared library at a different address than where they are mapped
in the backend that requests the new process to be started. However,
that doesn't really matter; AFAICS, you can just as well call pqsignal
to set the handlers to anything you want from the main entrypoint
before unblocking signals. So I'm inclined to say we should just
remove bgw_sighup and bgw_sigterm altogether and tell people to do it
that way.

Alternatively, we could give them the same treatment that I gave
bgw_main: let the user specify a function name and we'll search the
appropriate DSO for it. But that's probably less convenient for
anyone using this facility than just calling pqsignal() before
unblocking signals, so I don't see any real reason to go that route.

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