Fix parallel pg_dump/pg_restore for failure to create worker processes.

tglsfdc · tglsfdc · commit 5d60df8306c8 · 2020-01-31T14:41:49.000-05:00
If we failed to fork a worker process, or create a communication pipe for one, WaitForTerminatingWorkers would suffer an assertion failure if assert-enabled, otherwise crash or go into an infinite loop. This was a consequence of not accounting for the startup condition where we've not yet forked all the workers. The original bug was that ParallelBackupStart would set workerStatus to WRKR_IDLE before it had successfully forked a worker. I made things worse in commit b7b8cc0 by not understanding the undocumented fact that the WRKR_TERMINATED state was also meant to represent the case where a worker hadn't been started yet: I changed enum T_WorkerStatus so that *all* the worker slots were initially in WRKR_IDLE state. But this wasn't any more broken in practice, since even one slot in the wrong state would keep WaitForTerminatingWorkers from terminating. In v10 and later, introduce an explicit T_WorkerStatus value for worker-not-started, in hopes of preventing future oversights of the same ilk. Before that, just document that WRKR_TERMINATED is supposed to cover that case (partly because it wasn't actively broken, and partly because the enum is exposed outside parallel.c in those branches, so there's microscopically more risk involved in changing it). In all branches, introduce a WORKER_IS_RUNNING status test macro to hide which T_WorkerStatus values mean that, and be more careful not to access ParallelSlot fields till we're sure they're valid. Per report from Vignesh C, though this is my patch not his. Back-patch to all supported branches. Discussion: https://postgr.es/m/CALDaNm1Luv-E3sarR+-unz-BjchquHHyfP+YC+2FS2pt_J+wxg@mail.gmail.com
diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c
@@ -50,7 +50,7 @@
  *		WRKR_IDLE: it's waiting for a command
  *		WRKR_WORKING: it's been sent a command
  *		WRKR_FINISHED: it's returned a result
- *		WRKR_TERMINATED: process ended
+ *		WRKR_TERMINATED: process ended (or not started yet)
  * The FINISHED state indicates that the worker is idle, but we've not yet
  * dealt with the status code it returned from the prior command.
  * ReapWorkerStatus() extracts the unhandled command status value and sets
@@ -381,7 +381,9 @@ ShutdownWorkersHard(ParallelState *pstate)
 
 	/*
 	 * Close our write end of the sockets so that any workers waiting for
-	 * commands know they can exit.
+	 * commands know they can exit.  (Note: some of the pipeWrite fields might
+	 * still be zero, if we failed to initialize all the workers.  Hence, just
+	 * ignore errors here.)
 	 */
 	for (i = 0; i < pstate->numWorkers; i++)
 		closesocket(pstate->parallelSlot[i].pipeWrite);
@@ -455,7 +457,7 @@ WaitForTerminatingWorkers(ParallelState *pstate)
 
 		for (j = 0; j < pstate->numWorkers; j++)
 		{
-			if (pstate->parallelSlot[j].workerStatus != WRKR_TERMINATED)
+			if (WORKER_IS_RUNNING(pstate->parallelSlot[j].workerStatus))
 			{
 				lpHandles[nrun] = (HANDLE) pstate->parallelSlot[j].hThread;
 				nrun++;
@@ -891,6 +893,7 @@ ParallelBackupStart(ArchiveHandle *AH, RestoreOptions *ropt)
 	if (AH->public.numWorkers == 1)
 		return pstate;
 
+	/* Create status array, being sure to initialize all fields to 0 */
 	pstate->parallelSlot = (ParallelSlot *) pg_malloc(slotSize);
 	memset((void *) pstate->parallelSlot, 0, slotSize);
 
@@ -932,17 +935,16 @@ ParallelBackupStart(ArchiveHandle *AH, RestoreOptions *ropt)
 		int			pipeMW[2],
 					pipeWM[2];
 
+		slot->args = (ParallelArgs *) pg_malloc(sizeof(ParallelArgs));
+		slot->args->AH = NULL;
+		slot->args->te = NULL;
+
 		/* Create communication pipes for this worker */
 		if (pgpipe(pipeMW) < 0 || pgpipe(pipeWM) < 0)
 			exit_horribly(modulename,
 						  "could not create communication channels: %s\n",
 						  strerror(errno));
 
-		slot->workerStatus = WRKR_IDLE;
-		slot->args = (ParallelArgs *) pg_malloc(sizeof(ParallelArgs));
-		slot->args->AH = NULL;
-		slot->args->te = NULL;
-
 		/* master's ends of the pipes */
 		slot->pipeRead = pipeWM[PIPE_READ];
 		slot->pipeWrite = pipeMW[PIPE_WRITE];
@@ -961,6 +963,7 @@ ParallelBackupStart(ArchiveHandle *AH, RestoreOptions *ropt)
 		handle = _beginthreadex(NULL, 0, (void *) &init_spawned_worker_win32,
 								wi, 0, &(slot->threadId));
 		slot->hThread = handle;
+		slot->workerStatus = WRKR_IDLE;
 #else							/* !WIN32 */
 		pid = fork();
 		if (pid == 0)
@@ -1005,6 +1008,7 @@ ParallelBackupStart(ArchiveHandle *AH, RestoreOptions *ropt)
 
 		/* In Master after successful fork */
 		slot->pid = pid;
+		slot->workerStatus = WRKR_IDLE;
 
 		/* close read end of Master -> Worker */
 		closesocket(pipeMW[PIPE_READ]);
@@ -1121,7 +1125,7 @@ GetIdleWorker(ParallelState *pstate)
 }
 
 /*
- * Return true iff every worker is in the WRKR_TERMINATED state.
+ * Return true iff no worker is running.
  */
 static bool
 HasEveryWorkerTerminated(ParallelState *pstate)
@@ -1130,7 +1134,7 @@ HasEveryWorkerTerminated(ParallelState *pstate)
 
 	for (i = 0; i < pstate->numWorkers; i++)
 	{
-		if (pstate->parallelSlot[i].workerStatus != WRKR_TERMINATED)
+		if (WORKER_IS_RUNNING(pstate->parallelSlot[i].workerStatus))
 			return false;
 	}
 	return true;
@@ -1530,7 +1534,7 @@ getMessageFromWorker(ParallelState *pstate, bool do_wait, int *worker)
 	FD_ZERO(&workerset);
 	for (i = 0; i < pstate->numWorkers; i++)
 	{
-		if (pstate->parallelSlot[i].workerStatus == WRKR_TERMINATED)
+		if (!WORKER_IS_RUNNING(pstate->parallelSlot[i].workerStatus))
 			continue;
 		FD_SET(pstate->parallelSlot[i].pipeRead, &workerset);
 		if (pstate->parallelSlot[i].pipeRead > maxFd)
@@ -1555,6 +1559,8 @@ getMessageFromWorker(ParallelState *pstate, bool do_wait, int *worker)
 	{
 		char	   *msg;
 
+		if (!WORKER_IS_RUNNING(pstate->parallelSlot[i].workerStatus))
+			continue;
 		if (!FD_ISSET(pstate->parallelSlot[i].pipeRead, &workerset))
 			continue;
 
diff --git a/src/bin/pg_dump/parallel.h b/src/bin/pg_dump/parallel.h
@@ -32,6 +32,9 @@ typedef enum
 	WRKR_FINISHED
 } T_WorkerStatus;
 
+#define WORKER_IS_RUNNING(workerStatus) \
+	((workerStatus) != WRKR_TERMINATED)
+
 /* Arguments needed for a worker process */
 typedef struct ParallelArgs
 {

Original file line number	Diff line number	Diff line change
`@@ -32,6 +32,9 @@ typedef enum`
`32`	`32`	`WRKR_FINISHED`
`33`	`33`	`} T_WorkerStatus;`
`34`	`34`
	`35`	`+#define WORKER_IS_RUNNING(workerStatus) \`
	`36`	`+ ((workerStatus) != WRKR_TERMINATED)`
	`37`	`+`
`35`	`38`	`/* Arguments needed for a worker process */`
`36`	`39`	`typedef struct ParallelArgs`
`37`	`40`	`{`