Re: Parallel pg_dump's error reporting doesn't work worth squat

How about silencing the workers on termination?

# Build on Windows (with VC?) is very time consuming...

At Fri, 03 Jun 2016 09:44:30 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <11515.1464961470@sss.pgh.pa.us>
> Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
> > For sure, any of the "dangers" of TerminateThread don't matter
> > for this case.
> 
> I think that this one:
> 
> >> If the target thread is allocating memory from the heap, the heap
> >> lock will not be released.
> 
> is potentially a hazard, which is why I made sure to use write_stderr
> later on in the console interrupt handler.  Your original suggestion
> to use write_msg would end up going through fprintf, which might well
> use malloc internally.  (It's possible that Windows' version of write()
> could too, I suppose, but that's probably as low-level as we are
> going to get.)

I have to admit that I forgot about the possible malloc's, and
PQcancel() can be blocked from the same reason. What is worse,
even if we managed to avoid this particular disaster, the MSDN
page says the problems are *for example*. So if we don't allow
any possible unknown blocking on ctrl-C termination and want to
forcibly terminate workers, we might have to use process instead
of thread for worker entity. (This might reduce the difference
related to WIN32..)

We also might be able to cause immediate process termination for
the second Ctrl-C but this seems to lead to other issues.

If the issue to be settled here is the unwanted error messages,
we could set a flag to instruct write_msg to sit silent. Anyway
the workers should have been dead that time so discarding any
error messages don't matter.

What do you think about this?


Timeout for select still seems to be needless.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c
index fb48a02..b74a396 100644
--- a/src/bin/pg_dump/parallel.c
+++ b/src/bin/pg_dump/parallel.c
@@ -147,7 +147,11 @@ static DWORD tls_index;/* globally visible variables (needed by exit_nicely) */bool
parallel_init_done= false;
 
-DWORD        mainThreadId;
+bool        is_terminating = false;
+static DWORD        mainThreadId;
+static bool            handler_active = false;
+static DWORD        handlerThreadId;
+#endif   /* WIN32 */static const char *modulename = gettext_noop("parallel archiver");
@@ -180,9 +184,26 @@ static char *readMessageFromPipe(int fd);/*
- * Shutdown callback to clean up socket access
+ * Return true if I am the main thread
+ *
+ * This function regards the console handler thread as the main thread. We
+ * need a separate boolean for validity of the handlerThreadId since an
+ * invalid value for thread id is not defined. */#ifdef WIN32
+bool
+am_main_thread(void)
+{
+  DWORD current_thread_id = GetCurrentThreadId();
+
+  return (mainThreadId == current_thread_id ||
+          (handler_active &&
+           handlerThreadId == current_thread_id));
+}
+
+/*
+ * Shutdown callback to clean up socket access
+ */static voidshutdown_parallel_dump_utils(int code, void *unused){
@@ -190,7 +211,7 @@ shutdown_parallel_dump_utils(int code, void *unused)    if (mainThreadId == GetCurrentThreadId())
    WSACleanup();}
 
-#endif
+#endif /* ifdef WIN32 *//* * Initialize parallel dump support --- should be called early in process
@@ -390,6 +411,8 @@ ShutdownWorkersHard(ParallelState *pstate)     * On Windows, send query cancels directly to the
workers'backends.  Use     * a critical section to ensure worker threads don't change state.     */
 
+    is_terminating = true; /* Silence workers */
+    EnterCriticalSection(&signal_info_lock);    for (i = 0; i < pstate->numWorkers; i++)    {
@@ -602,6 +625,15 @@ consoleHandler(DWORD dwCtrlType)    if (dwCtrlType == CTRL_C_EVENT ||        dwCtrlType ==
CTRL_BREAK_EVENT)   {
 
+          /* 
+         * We don't forcibly terminate workes. Instead, these
+         * variables make them silent ever after. This handler thread
+         * is regarded as the main thread.
+         */
+        is_terminating = true;
+        handlerThreadId = GetCurrentThreadId();
+        handler_active = true;
+        /* Critical section prevents changing data we look at here */        EnterCriticalSection(&signal_info_lock);
@@ -642,6 +674,8 @@ consoleHandler(DWORD dwCtrlType)        write_msg(NULL, "terminated by user\n");    }
+    handler_active = false;
+    /* Always return FALSE to allow signal handling to continue */    return FALSE;}
diff --git a/src/bin/pg_dump/parallel.h b/src/bin/pg_dump/parallel.h
index 21739ca..59fba33 100644
--- a/src/bin/pg_dump/parallel.h
+++ b/src/bin/pg_dump/parallel.h
@@ -64,11 +64,11 @@ typedef struct ParallelState#ifdef WIN32extern bool parallel_init_done;
-extern DWORD mainThreadId;
+extern bool is_terminating;#endifextern void init_parallel_dump_utils(void);
-
+extern bool am_main_thread(void);extern int    GetIdleWorker(ParallelState *pstate);extern bool
IsEveryWorkerIdle(ParallelState*pstate);extern void ListenToWorkers(ArchiveHandle *AH, ParallelState *pstate, bool
do_wait);
diff --git a/src/bin/pg_dump/pg_backup_utils.c b/src/bin/pg_dump/pg_backup_utils.c
index 01bf576..38c57bc 100644
--- a/src/bin/pg_dump/pg_backup_utils.c
+++ b/src/bin/pg_dump/pg_backup_utils.c
@@ -72,6 +72,15 @@ write_msg(const char *modulename, const char *fmt,...){    va_list        ap;
+#ifdef WIN32
+    /*
+     * On Windows, we don't forcibly terminate workers having ctrl-C
+     * entered. Instead, we sit silent.
+     */
+    if (is_terminating && !am_main_thread())
+      return;
+#endif
+    va_start(ap, fmt);    vwrite_msg(modulename, fmt, ap);    va_end(ap);
@@ -148,7 +157,7 @@ exit_nicely(int code)                                            on_exit_nicely_list[i].arg);#ifdef
WIN32
-    if (parallel_init_done && GetCurrentThreadId() != mainThreadId)
+    if (parallel_init_done && !am_main_thread())        _endthreadex(code);#endif