From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>
Cc: Magnus Hagander <magnus@hagander.net>
Cc: Mark Dilger <hornschnorter@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Windows buildfarm members vs. new async-notify isolation test
Date: Sun, 08 Dec 2019 11:22:08 -0500
Message-ID: <7634.1575822128@sss.pgh.pa.us> (raw)
In-Reply-To: <28011.1575816794@sss.pgh.pa.us>
References: <32745.1575303812@sss.pgh.pa.us>
<7221b3b4-fc7f-404d-5cf6-830d3aa6800d@2ndQuadrant.com>
<362bca0b-1c1c-c760-ab19-b5d9a14c69ea@gmail.com>
<13003.1575391248@sss.pgh.pa.us>
<CAA4eK1+qevZ=sB1nW05_gkiHLWjUU0psNPiutFnQi=+aCztLXA@mail.gmail.com>
<25746.1575436347@sss.pgh.pa.us>
<CAA8=A78X-0vVd5tk7r=Z7yEWW5Vy1=46q7wjZaOa8Xaed5FooQ@mail.gmail.com>
<CAA4eK1+HOFzs=-MHDz6ef936KuuHJBxZbHEi7xt9TBqgHNO9GQ@mail.gmail.com>
<e1eb7182-a487-42f1-e113-4e5758e8ceef@2ndQuadrant.com>
<20102.1575675105@sss.pgh.pa.us>
<CAA4eK1LRwxr51JuXsgpmCA+-uswCHiGvhZ88jPz8VeJxfyv65g@mail.gmail.com>
<30330.1575739251@sss.pgh.pa.us>
<4412.1575748586@sss.pgh.pa.us>
<CAA4eK1KpRMRJG0krbiL8sUA9wZTVwvoHejEkJK2sVH2idG-rSQ@mail.gmail.com>
<CAA4eK1+eo0t20xkPX9-WGdO+tDwEv9krba-R2F3OSqyLAWW59w@mail.gmail.com>
<28011.1575816794@sss.pgh.pa.us>
I wrote:
> Amit Kapila <amit.kapila16@gmail.com> writes:
>> IIUC, once the dispatch thread has queued the signal
>> (pg_queue_signal), the next CHECK_FOR_INTERRUPTS by the main thread
>> will execute the signal. So, if we move pg_queue_signal() before we
>> do WriteFile in pg_signal_dispatch_thread(), this race condition will
>> be closed. Now, we might not want to do this as that will add some
>> more time (even though very less) before notify on the other side can
>> finish or maybe there is some technical problem with this idea which I
>> am not able to see immediately.
> Hmm. Certainly worth trying to see if it resolves the failure on
> Andrew's machines.
For Andrew's convenience, here's a draft patch for that. I took the
liberty of improving the rather thin comments in this area, too.
regards, tom lane
Attachments:
[text/x-diff] this-might-fix-windows-race-condition.patch (2.8K, ../7634.1575822128@sss.pgh.pa.us/2-this-might-fix-windows-race-condition.patch)
download | inline diff:diff --git a/src/backend/port/win32/signal.c b/src/backend/port/win32/signal.cindex 9b5e544..f1e35fd 100644--- a/src/backend/port/win32/signal.c+++ b/src/backend/port/win32/signal.c@@ -208,12 +208,15 @@ pgwin32_create_signal_listener(pid_t pid)
*/
+/*+ * Queue a signal for the main thread, by setting the flag bit and event.+ */
void
pg_queue_signal(int signum)
{
Assert(pgwin32_signal_event != NULL);
if (signum >= PG_SIGNAL_COUNT || signum <= 0)
- return;+ return; /* ignore any bad signal number */
EnterCriticalSection(&pg_signal_crit_sec);
pg_signal_queue |= sigmask(signum);
@@ -222,7 +225,11 @@ pg_queue_signal(int signum)
SetEvent(pgwin32_signal_event);
}
-/* Signal dispatching thread */+/*+ * Signal dispatching thread. This runs after we have received a named+ * pipe connection from a client (signal sender). Process the request,+ * close the pipe, and exit.+ */
static DWORD WINAPI
pg_signal_dispatch_thread(LPVOID param)
{
@@ -242,13 +249,37 @@ pg_signal_dispatch_thread(LPVOID param)
CloseHandle(pipe);
return 0;
}
++ /*+ * Queue the signal before responding to the client. In this way, it's+ * guaranteed that once kill() has returned in the signal sender, the next+ * CHECK_FOR_INTERRUPTS() in the signal recipient will see the signal.+ * (This is a stronger guarantee than POSIX makes; maybe we don't need it?+ * But without it, we've seen timing bugs on Windows that do not manifest+ * on any known Unix.)+ */+ pg_queue_signal(sigNum);++ /*+ * Write something back to the client, allowing its CallNamedPipe() call+ * to terminate.+ */
WriteFile(pipe, &sigNum, 1, &bytes, NULL); /* Don't care if it works or
* not.. */
++ /*+ * We must wait for the client to read the data before we can close the+ * pipe, else the data will be lost. (If the WriteFile call failed,+ * there'll be nothing in the buffer, so this shouldn't block.)+ */
FlushFileBuffers(pipe);
++ /* This is a formality, since we're about to close the pipe anyway. */
DisconnectNamedPipe(pipe);
++ /* And we're done. */
CloseHandle(pipe);
- pg_queue_signal(sigNum);
return 0;
}
@@ -266,6 +297,7 @@ pg_signal_thread(LPVOID param)
BOOL fConnected;
HANDLE hThread;
+ /* Create a new pipe instance if we don't have one. */
if (pipe == INVALID_HANDLE_VALUE)
{
pipe = CreateNamedPipe(pipename, PIPE_ACCESS_DUPLEX,
@@ -280,6 +312,11 @@ pg_signal_thread(LPVOID param)
}
}
+ /*+ * Wait for a client to connect. If something connects before we+ * reach here, we'll get back a "failure" with ERROR_PIPE_CONNECTED,+ * which is actually a success (way to go, Microsoft).+ */
fConnected = ConnectNamedPipe(pipe, NULL) ? TRUE : (GetLastError() == ERROR_PIPE_CONNECTED);
if (fConnected)
{
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, andrew.dunstan@2ndquadrant.com, amit.kapila16@gmail.com, magnus@hagander.net, hornschnorter@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Windows buildfarm members vs. new async-notify isolation test
In-Reply-To: <7634.1575822128@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox