Re: BUG #16161: pg_ctl stop fails sometimes (on Windows)

19.12.2019 4:39, Kyotaro Horiguchi wrote:
> At Wed, 18 Dec 2019 17:09:15 -0500, Tom Lane <[email protected]> wrote in 
>> Alexander Lakhin <[email protected]> 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

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;
+				}
 			}
 		}