Re: [HACKERS] ParallelContexts can get confused about which worker is which
On Tue, Nov 3, 2015 at 11:07 PM, Amit Kapila wrote: > On Tue, Nov 3, 2015 at 8:25 PM, Robert Haas wrote: >> On Mon, Nov 2, 2015 at 4:13 PM, Robert Haas wrote: >> > On Sun, Nov 1, 2015 at 1:11 AM, Amit Kapila >> > wrote: >> >> If we are going to add a new parameter to BackgroundWorker structure, >> >> then the same needs to be updated in docs [1] as well. >> > >> > Right, good point. >> >> Here's an updated patch with documentation added and two bugs fixed. >> > > Patch looks good to me. OK, committed. -- 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
Re: [HACKERS] ParallelContexts can get confused about which worker is which
On Tue, Nov 3, 2015 at 8:25 PM, Robert Haas wrote: > > On Mon, Nov 2, 2015 at 4:13 PM, Robert Haas wrote: > > On Sun, Nov 1, 2015 at 1:11 AM, Amit Kapila wrote: > >> If we are going to add a new parameter to BackgroundWorker structure, > >> then the same needs to be updated in docs [1] as well. > > > > Right, good point. > > Here's an updated patch with documentation added and two bugs fixed. > Patch looks good to me. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] ParallelContexts can get confused about which worker is which
On Mon, Nov 2, 2015 at 4:13 PM, Robert Haas wrote:
> On Sun, Nov 1, 2015 at 1:11 AM, Amit Kapila wrote:
>> If we are going to add a new parameter to BackgroundWorker structure,
>> then the same needs to be updated in docs [1] as well.
>
> Right, good point.
Here's an updated patch with documentation added and two bugs fixed.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/doc/src/sgml/bgworker.sgml b/doc/src/sgml/bgworker.sgml
index c17d398..505e362 100644
--- a/doc/src/sgml/bgworker.sgml
+++ b/doc/src/sgml/bgworker.sgml
@@ -58,6 +58,7 @@ typedef struct BackgroundWorker
charbgw_library_name[BGW_MAXLEN]; /* only if bgw_main is NULL */
charbgw_function_name[BGW_MAXLEN]; /* only if bgw_main is NULL */
Datum bgw_main_arg;
+charbgw_extra[BGW_EXTRALEN];
int bgw_notify_pid;
} BackgroundWorker;
@@ -183,6 +184,13 @@ typedef struct BackgroundWorker
+ bgw_extra can contain extra data to be passed
+ to the background worker. Unlike bgw_main_arg, this data
+ is not passed as an argument to the worker's main function, but it can be
+ accessed via MyBgworkerEntry, as discussed above.
+
+
+
bgw_notify_pid is the PID of a PostgreSQL
backend process to which the postmaster should send SIGUSR1
when the process is started or exits. It should be 0 for workers registered
diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c
index 7cb53f2..ab00279 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -77,10 +77,6 @@ typedef struct FixedParallelState
/* Mutex protects remaining fields. */
slock_t mutex;
- /* Track whether workers have attached. */
- int workers_expected;
- int workers_attached;
-
/* Maximum XactLastRecEnd of any worker. */
XLogRecPtr last_xlog_end;
} FixedParallelState;
@@ -295,8 +291,6 @@ InitializeParallelDSM(ParallelContext *pcxt)
fps->parallel_master_backend_id = MyBackendId;
fps->entrypoint = pcxt->entrypoint;
SpinLockInit(&fps->mutex);
- fps->workers_expected = pcxt->nworkers;
- fps->workers_attached = 0;
fps->last_xlog_end = 0;
shm_toc_insert(pcxt->toc, PARALLEL_KEY_FIXED, fps);
@@ -403,7 +397,6 @@ ReinitializeParallelDSM(ParallelContext *pcxt)
/* Reset a few bits of fixed parallel state to a clean state. */
fps = shm_toc_lookup(pcxt->toc, PARALLEL_KEY_FIXED);
- fps->workers_attached = 0;
fps->last_xlog_end = 0;
/* Recreate error queues. */
@@ -455,6 +448,7 @@ LaunchParallelWorkers(ParallelContext *pcxt)
worker.bgw_main = ParallelWorkerMain;
worker.bgw_main_arg = UInt32GetDatum(dsm_segment_handle(pcxt->seg));
worker.bgw_notify_pid = MyProcPid;
+ memset(&worker.bgw_extra, 0, BGW_EXTRALEN);
/*
* Start workers.
@@ -466,6 +460,7 @@ LaunchParallelWorkers(ParallelContext *pcxt)
*/
for (i = 0; i < pcxt->nworkers; ++i)
{
+ memcpy(worker.bgw_extra, &i, sizeof(int));
if (!any_registrations_failed &&
RegisterDynamicBackgroundWorker(&worker,
&pcxt->worker[i].bgwhandle))
@@ -893,6 +888,10 @@ ParallelWorkerMain(Datum main_arg)
pqsignal(SIGTERM, die);
BackgroundWorkerUnblockSignals();
+ /* Determine and set our parallel worker number. */
+ Assert(ParallelWorkerNumber == -1);
+ memcpy(&ParallelWorkerNumber, MyBgworkerEntry->bgw_extra, sizeof(int));
+
/* Set up a memory context and resource owner. */
Assert(CurrentResourceOwner == NULL);
CurrentResourceOwner = ResourceOwnerCreate(NULL, "parallel toplevel");
@@ -917,18 +916,9 @@ ParallelWorkerMain(Datum main_arg)
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("bad magic number in dynamic shared memory segment")));
- /* Determine and set our worker number. */
+ /* Look up fixed parallel state. */
fps = shm_toc_lookup(toc, PARALLEL_KEY_FIXED);
Assert(fps != NULL);
- Assert(ParallelWorkerNumber == -1);
- SpinLockAcquire(&fps->mutex);
- if (fps->workers_attached < fps->workers_expected)
- ParallelWorkerNumber = fps->workers_attached++;
- SpinLockRelease(&fps->mutex);
- if (ParallelWorkerNumber < 0)
- ereport(ERROR,
-(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("too many parallel workers already attached")));
MyFixedParallelState = fps;
/*
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index cf505d6..2a878eb 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -314,6 +314,7 @@ BackgroundWorkerStateChange(void)
rw->rw_worker.bgw_restart_time = slot->worker.bgw_restart_time;
rw->rw_worker.bgw_main = slot->worker.bgw_main;
rw->rw_worker.bgw_main_arg = slot->worker.bgw_main_arg;
+ memcpy(rw->rw_worker.bgw_extra, slot->worker.bgw_extra, BGW_EXTRALEN);
/*
* Copy the PID to be notified about state changes, but only if the
diff --git a/src/include/postmaster/bgworker.h b/src/include/postmaster/bgworker.h
Re: [HACKERS] ParallelContexts can get confused about which worker is which
On Sun, Nov 1, 2015 at 1:11 AM, Amit Kapila wrote: > If we are going to add a new parameter to BackgroundWorker structure, > then the same needs to be updated in docs [1] as well. Right, good point. > I think adding > a new parameter to this structure might require some updations in > client applications. It seems worth to add a note for the same in commit > message, so that same could be reflected in Release Notes. Client background worker modules need to be recompiled so that they know about the new structure size, but they shouldn't need any code changes. > Also, I don't know why BGW_EXTRALEN needs to be 128 and not 64? > I guess you kept it so that in future if we need to pass more information, > then the same could be used which seems reasonable considering that > this is an exposed structure and we don't want to change it again. Well, I only need 4 bytes for this purpose, but I figured other users of the background worker facility might like to have a little more. I don't think 64 vs. 128 matters very much; I'm happy to hear other opinions on this topic. What I think mostly matters is that we don't make it so much that the overhead of copying it would add substantially to background worker startup times. I assume that either 64 or 128 is safe in that regard. > /* Mutex protects remaining fields. */ > slock_t mutex; > > > /* Maximum XactLastRecEnd of any worker. */ > XLogRecPtr last_xlog_end; > > } FixedParallelState; > > Now as above mutex will be used to just protect last_xlog_end, do you > think it is better to modify the comment above mutex declaration? Meh. -- 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
Re: [HACKERS] ParallelContexts can get confused about which worker is which
On Sat, Oct 31, 2015 at 11:35 AM, Robert Haas wrote: > > On Fri, Oct 30, 2015 at 11:24 AM, Robert Haas wrote: > > The other way to fix this is to pass down the index > > that the leader assigns to any given worker, and have the worker use > > that index instead of allocating its own separate index after > > connecting to the DSM segment. Unfortunately, there's not really a > > good way to pass that additional information down to the worker right > > now, but we could fix that pretty easily by adding an additional field > > to the BackgroundWorker structure, which the worker would then be able > > to access via MyBgworkerEntry. > > Here's a patch implementing that. > If we are going to add a new parameter to BackgroundWorker structure, then the same needs to be updated in docs [1] as well. I think adding a new parameter to this structure might require some updations in client applications. It seems worth to add a note for the same in commit message, so that same could be reflected in Release Notes. Also, I don't know why BGW_EXTRALEN needs to be 128 and not 64? I guess you kept it so that in future if we need to pass more information, then the same could be used which seems reasonable considering that this is an exposed structure and we don't want to change it again. /* Mutex protects remaining fields. */ slock_t mutex; /* Maximum XactLastRecEnd of any worker. */ XLogRecPtr last_xlog_end; } FixedParallelState; Now as above mutex will be used to just protect last_xlog_end, do you think it is better to modify the comment above mutex declaration? [1] - http://www.postgresql.org/docs/devel/static/bgworker.html With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] ParallelContexts can get confused about which worker is which
On Fri, Oct 30, 2015 at 11:24 AM, Robert Haas wrote:
> The other way to fix this is to pass down the index
> that the leader assigns to any given worker, and have the worker use
> that index instead of allocating its own separate index after
> connecting to the DSM segment. Unfortunately, there's not really a
> good way to pass that additional information down to the worker right
> now, but we could fix that pretty easily by adding an additional field
> to the BackgroundWorker structure, which the worker would then be able
> to access via MyBgworkerEntry.
Here's a patch implementing that.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c
index 79cc988..c073a42 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -77,10 +77,6 @@ typedef struct FixedParallelState
/* Mutex protects remaining fields. */
slock_t mutex;
- /* Track whether workers have attached. */
- int workers_expected;
- int workers_attached;
-
/* Maximum XactLastRecEnd of any worker. */
XLogRecPtr last_xlog_end;
} FixedParallelState;
@@ -295,8 +291,6 @@ InitializeParallelDSM(ParallelContext *pcxt)
fps->parallel_master_backend_id = MyBackendId;
fps->entrypoint = pcxt->entrypoint;
SpinLockInit(&fps->mutex);
- fps->workers_expected = pcxt->nworkers;
- fps->workers_attached = 0;
fps->last_xlog_end = 0;
shm_toc_insert(pcxt->toc, PARALLEL_KEY_FIXED, fps);
@@ -403,7 +397,6 @@ ReinitializeParallelDSM(ParallelContext *pcxt)
/* Reset a few bits of fixed parallel state to a clean state. */
fps = shm_toc_lookup(pcxt->toc, PARALLEL_KEY_FIXED);
- fps->workers_attached = 0;
fps->last_xlog_end = 0;
/* Recreate error queues. */
@@ -455,6 +448,7 @@ LaunchParallelWorkers(ParallelContext *pcxt)
worker.bgw_main = ParallelWorkerMain;
worker.bgw_main_arg = UInt32GetDatum(dsm_segment_handle(pcxt->seg));
worker.bgw_notify_pid = MyProcPid;
+ memset(worker.bgw_extra, 0, BGW_EXTRALEN);
/*
* Start workers.
@@ -466,6 +460,7 @@ LaunchParallelWorkers(ParallelContext *pcxt)
*/
for (i = 0; i < pcxt->nworkers; ++i)
{
+ memcpy(worker.bgw_extra, &i, sizeof(int));
if (!any_registrations_failed &&
RegisterDynamicBackgroundWorker(&worker,
&pcxt->worker[i].bgwhandle))
@@ -891,6 +886,10 @@ ParallelWorkerMain(Datum main_arg)
pqsignal(SIGTERM, die);
BackgroundWorkerUnblockSignals();
+ /* Determine and set our parallel worker number. */
+ Assert(ParallelWorkerNumber == -1);
+ memcpy(&ParallelWorkerNumber, MyBgworkerEntry->bgw_extra, sizeof(int));
+
/* Set up a memory context and resource owner. */
Assert(CurrentResourceOwner == NULL);
CurrentResourceOwner = ResourceOwnerCreate(NULL, "parallel toplevel");
@@ -915,18 +914,9 @@ ParallelWorkerMain(Datum main_arg)
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("bad magic number in dynamic shared memory segment")));
- /* Determine and set our worker number. */
+ /* Look up fixed parallel state. */
fps = shm_toc_lookup(toc, PARALLEL_KEY_FIXED);
Assert(fps != NULL);
- Assert(ParallelWorkerNumber == -1);
- SpinLockAcquire(&fps->mutex);
- if (fps->workers_attached < fps->workers_expected)
- ParallelWorkerNumber = fps->workers_attached++;
- SpinLockRelease(&fps->mutex);
- if (ParallelWorkerNumber < 0)
- ereport(ERROR,
-(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("too many parallel workers already attached")));
MyFixedParallelState = fps;
/*
diff --git a/src/include/postmaster/bgworker.h b/src/include/postmaster/bgworker.h
index f0a9530..6e0b5cd 100644
--- a/src/include/postmaster/bgworker.h
+++ b/src/include/postmaster/bgworker.h
@@ -74,6 +74,7 @@ typedef enum
#define BGW_DEFAULT_RESTART_INTERVAL 60
#define BGW_NEVER_RESTART-1
#define BGW_MAXLEN 64
+#define BGW_EXTRALEN 128
typedef struct BackgroundWorker
{
@@ -85,6 +86,7 @@ typedef struct BackgroundWorker
char bgw_library_name[BGW_MAXLEN]; /* only if bgw_main is NULL */
char bgw_function_name[BGW_MAXLEN]; /* only if bgw_main is NULL */
Datum bgw_main_arg;
+ char bgw_extra[BGW_EXTRALEN];
pid_t bgw_notify_pid; /* SIGUSR1 this backend on start/stop */
} BackgroundWorker;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
