Subject: Re: BUG #16161: pg_ctl stop fails sometimes (on Windows)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, tgl@sss.pgh.pa.us
Cc: pgsql-bugs@lists.postgresql.org
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>
From: Alexander Lakhin <exclusion@gmail.com>
Message-ID: <d32a75e6-ba99-c95c-e475-1887c009d395@gmail.com>
Date: Thu, 19 Dec 2019 07:00:00 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.2
MIME-Version: 1.0
In-Reply-To: <20191219.103948.2182167132387989763.horikyota.ntt@gmail.com>
Content-Type: multipart/mixed;
 boundary="------------2DEAC356BBAF8C89B2F2C5AF"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------2DEAC356BBAF8C89B2F2C5AF
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit

19.12.2019 4:39, Kyotaro Horiguchi wrote:
> At Wed, 18 Dec 2019 17:09:15 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
>> Alexander Lakhin <exclusion@gmail.com> writes:
>>> So I see three ways now:
>>> 1) Revert the pgwin32_open change and choose the previously rejected
>>> approach: Unlink postmaster.pid using rename operation (adopt the
>>> solution from https://stackoverflow.com/questions/3764072/).
>>> 2) Add a check for directory in pgwin32_open, but I think then we're
>>> getting close to checking there just for postmaster.pid.
>>> 3) Find out how often we get ERROR_ACCESS_DENIED during all regression
>>> tests and if such error count is near zero, then add the check for isdir
>>> in fsync_fname_ext and friends, and get done.
>> I'm not terribly thrilled with (1) because I do not think that
>> postmaster.pid accesses are the only place where we have this issue.
>> And (3) is not only not a forward-looking solution, but it imagines
>> that the speed of the regression tests is the only thing we need to
>> worry about here.  We generally don't assume that the regression
>> tests are an accurate model of the performance users will see.
>>
>> So that leaves us with (2), it seems.
> Agreed for (2).
>
> I'm not sure about the point of the discussion that stat()'ing is
> messy. If it's about performance, does stat()ing only when CreateFile
> returned ERROR_ACCESS_DENIED helps? Anyway we wait for a moment in
> that case.
Please look at the patch that implements (2). It makes `vcregress
recoverycheck` pass (and my demo restart test still passes too).
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.

Best regards,
Alexander

--------------2DEAC356BBAF8C89B2F2C5AF
Content-Type: text/x-patch;
 name="improve-eacces-handling.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="improve-eacces-handling.patch"

diff --git a/src/port/open.c b/src/port/open.c
index 51652762a1..2196cbc992 100644
--- a/src/port/open.c
+++ b/src/port/open.c
@@ -144,11 +144,19 @@ pgwin32_open(const char *fileName, int fileFlags,...)
 		 */
 		if (err == ERROR_ACCESS_DENIED)
 		{
-			if (loops < 10)
-			{
-				pg_usleep(100000);
-				loops++;
-				continue;
+			/*
+			 * We don't want to retry fopen() with a directory,
+			 * which is known to fail with the same ERROR_ACCESS_DENIED error
+			 * (Windows NT status code STATUS_FILE_IS_A_DIRECTORY).
+			 */
+			struct stat st;
+			if (stat(fileName, &st) != 0 || !S_ISDIR(st.st_mode)) {
+				if (loops < 10)
+				{
+					pg_usleep(100000);
+					loops++;
+					continue;
+				}
 			}
 		}
 

--------------2DEAC356BBAF8C89B2F2C5AF--