From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alexander Lakhin <exclusion@gmail.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16161: pg_ctl stop fails sometimes (on Windows)
Date: Thu, 19 Dec 2019 10:44:08 -0500
Message-ID: <32374.1576770248@sss.pgh.pa.us> (raw)
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>
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
Attachments:
[text/x-diff] improve-eacces-handling-2.patch (1.8K, ../32374.1576770248@sss.pgh.pa.us/2-improve-eacces-handling-2.patch)
download | inline diff:diff --git a/src/port/open.c b/src/port/open.cindex 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;+ }
}
}
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-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, exclusion@gmail.com, horikyota.ntt@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16161: pg_ctl stop fails sometimes (on Windows)
In-Reply-To: <32374.1576770248@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