Re: [HACKERS] ParallelContexts can get confused about which worker is which

[Robert Haas]

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

[Amit Kapila]

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

[Robert Haas]

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(>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(_extra, 0, BGW_EXTRALEN);
 
 	/*
 	 * Start workers.
@@ -466,6 +460,7 @@ LaunchParallelWorkers(ParallelContext *pcxt)
 	 */
 	for (i = 0; i < pcxt->nworkers; ++i)
 	{
+		memcpy(worker.bgw_extra, , sizeof(int));
 		if (!any_registrations_failed &&
 			RegisterDynamicBackgroundWorker(,
 			>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(, 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(>mutex);
-	if (fps->workers_attached < fps->workers_expected)
-		ParallelWorkerNumber = fps->workers_attached++;
-	SpinLockRelease(>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
index 

Re: [HACKERS] ParallelContexts can get confused about which worker is which

[Robert Haas]

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

[Amit Kapila]

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

[Robert Haas]

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(>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, , sizeof(int));
 		if (!any_registrations_failed &&
 			RegisterDynamicBackgroundWorker(,
 			>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(, 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(>mutex);
-	if (fps->workers_attached < fps->workers_expected)
-		ParallelWorkerNumber = fps->workers_attached++;
-	SpinLockRelease(>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

[HACKERS] ParallelContexts can get confused about which worker is which

[Robert Haas]

While testing last night, I discovered a serious case of brain fade in
parallel.c; the same conceptual mistake has also spread to
nodeGather.c.  parallel.c creates an array of ParallelWorkerInfo
structures, which are defined like this:

typedef struct ParallelWorkerInfo
{
BackgroundWorkerHandle *bgwhandle;
shm_mq_handle *error_mqh;
int32   pid;
} ParallelWorkerInfo;

These structures are typically accessed as pcxt->worker[i].  The
trouble is that pcxt->worker[i].bgwhandle is the bgwhandler for the
i'th worker to be registered, while pcxt->worker[i].error_mqh is the
error_mqh for the i'th worker to attach to the dynamic shared memory
segment.  But those might be totally different processes.

This happens to mostly work, because the postmaster will probably
start the processes in the same order that they are registered, and
the operating system will probably schedule them in the same order the
postmaster starts them.  And if you only have one worker, then you're
fine!  It also seems to work out that if all workers exit cleanly, any
shuffling of the background worker handles relative to the error queue
handles is harmless.  But it's still pretty broken.

There seem to be two ways to fix this.  One is to keep the bgwhandle
objects in a separate array from the error_mqh objects and reconstruct
after the fact what the mapping between those two sets of indexes is.
This solution looks complicated to code and generally pretty annoying
to get right.  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.  I suggest something like this:

--- 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
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];
pid_t   bgw_notify_pid; /* SIGUSR1 this backend on start/stop */
 } BackgroundWorker;

The ability to pass down a little more information from the
registering process to the background worker seems like it would be
useful for more than just parallelism, so I imagine this change might
be generally welcomed.  Parallel workers would use the first four
bytes of bgw_extra to store the worker index, and other users of the
background worker facility could use it for whatever they like.

Comments?

-- 
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