From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alexander Lakhin <exclusion@gmail.com>
cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
        pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16161: pg_ctl stop fails sometimes (on Windows)
In-reply-to: <d32a75e6-ba99-c95c-e475-1887c009d395@gmail.com>
References: <4741.1576684991@sss.pgh.pa.us>
 <b2cc98b1-0ba0-1358-572f-8a1a01a3880e@gmail.com>
 <26441.1576706955@sss.pgh.pa.us>
 <20191219.103948.2182167132387989763.horikyota.ntt@gmail.com>
 <d32a75e6-ba99-c95c-e475-1887c009d395@gmail.com>
Comments: In-reply-to Alexander Lakhin <exclusion@gmail.com>
	message dated "Thu, 19 Dec 2019 07:00:00 +0300"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <32193.1576769987.0@sss.pgh.pa.us>
Date: Thu, 19 Dec 2019 10:44:08 -0500
Message-ID: <32374.1576770248@sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <32193.1576769987.1@sss.pgh.pa.us>

Alexander Lakhin <exclusion@gmail.com> writes:
> Please look at the patch that implements (2). It makes `vcregress
> recoverycheck` pass (and my demo restart test still passes too).

I think we could be a little smarter here.  If the problem is
STATUS_DELETE_PENDING, doesn't that affect stat() as well?  That is,
if stat() succeeds, we needn't wait, independently of whether it
says S_ISDIR or not?  This seems like a noticeable improvement if
true, because it would mean that ordinary file-permission failures
also need not wait.  We'd still get confused if we get a permission
failure on a containing directory rather than the file itself, but
that I'm prepared to dismiss as an uncommon case.

Entirely-untested patch along this line attached.

BTW ... it's likely that stat() here is actually going to invoke
pgwin32_safestat(), which has its own opinions about this, and
indeed seems to think we'll get ERROR_DELETE_PENDING.  I think
that's harmless here, but it makes me wonder if we should use
a native Windows API instead of going through stat().

> As open(dir) is getting a little more expensive than before, maybe it's
> still worthwhile to patch fsync*(..., isdir,...). I can prepare such a
> patch if so.

I think we should leave that for later, so that the buildfarm can
actually test whatever logic we put in here.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="improve-eacces-handling-2.patch";
	charset="us-ascii"
Content-ID: <32193.1576769987.2@sss.pgh.pa.us>
Content-Description: improve-eacces-handling-2.patch

diff --git a/src/port/open.c b/src/port/open.c
index 5165276..24af6f4 100644
--- a/src/port/open.c
+++ b/src/port/open.c
@@ -21,6 +21,7 @@
 
 #include <fcntl.h>
 #include <assert.h>
+#include <sys/stat.h>
 
 
 static int
@@ -137,18 +138,33 @@ pgwin32_open(const char *fileName, int fileFlags,...)
 		}
 
 		/*
-		 * ERROR_ACCESS_DENIED can be returned if the file is deleted but not
-		 * yet gone (Windows NT status code is STATUS_DELETE_PENDING).  Wait a
-		 * bit and try again, giving up after 1 second (since this condition
-		 * should never persist very long).
+		 * ERROR_ACCESS_DENIED is returned if the file is deleted but not yet
+		 * gone (Windows NT status code is STATUS_DELETE_PENDING).  In that
+		 * case we want to wait a bit and try again, giving up after 1 second
+		 * (since this condition should never persist very long).  However,
+		 * there are other commonly-hit cases that return ERROR_ACCESS_DENIED,
+		 * so care is needed.  In particular that happens if we try to open a
+		 * directory, or of course if there's an actual file-permissions
+		 * problem.  To distinguish these cases, try a stat().  In the
+		 * delete-pending case, it will either also get STATUS_DELETE_PENDING,
+		 * or it will see the file as gone and fail with ENOENT.  In other
+		 * cases it will usually succeed.  The only somewhat-likely case where
+		 * this coding will uselessly wait is if there's a permissions problem
+		 * with a containing directory, which we hope will never happen in any
+		 * performance-critical code paths.
 		 */
 		if (err == ERROR_ACCESS_DENIED)
 		{
 			if (loops < 10)
 			{
-				pg_usleep(100000);
-				loops++;
-				continue;
+				struct stat st;
+
+				if (stat(fileName, &st) != 0)
+				{
+					pg_usleep(100000);
+					loops++;
+					continue;
+				}
 			}
 		}
 

------- =_aaaaaaaaaa0--