15.11.2020 04:11, Justin Pryzby wrote:
> On Sat, Nov 14, 2020 at 01:00:00PM +0300, Alexander Lakhin wrote:
>> As noted in [1], a sensible solution would be putting the same "retry on
>> ERROR_ACCESS_DENIED" action in a wrapper for stat().
>> And bed90759f brought in master the _pgstat64() function, where such
>> error handling should be placed.
>> So please look at the patch (based on the previous one made to fix
>> bug#16161), that makes the attached test pass.
> Your patch introduces a "loops", but doesn't use it to escape the loop.
Indeed, this is my mistake. Please look at the corrected patch (now that
code corresponds to the pgwin32_open() as intended).
And it rises another question, what if pg_ls_dir_files() is called for a
directory where hundreds or thousands of files are really inaccessible
due to restrictions?
I mean that using CreateFile() in the modified stat() implementation can
be rather expensive for an arbitrary file (and worse yet, for many files).
On the positive side, for now pg_ls_dir_files() is called only from
pg_ls_logdir, pg_ls_waldir, pg_ls_tmpdir, pg_ls_archive_statusdir, where
having a bunch of files that are inaccessible for the postgres user is
not expected anyway.
But probably getting directory contents with correct file sizes (>4GB)
in pg_ls_dir_files() can be implemented without calling
CreateFile()/stat() at all (as ProcMon shows, the "dir" command doesn't
call CreateFile() (or any other system function) for each file in the
target directory).
Best regards,
Alexander
diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c
index d34182a7b0..922df49125 100644
--- a/src/backend/utils/adt/genfile.c
+++ b/src/backend/utils/adt/genfile.c
@@ -28,6 +28,7 @@
#include "funcapi.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
+#include "port.h"
#include "postmaster/syslogger.h"
#include "storage/fd.h"
#include "utils/acl.h"
diff --git a/src/port/win32stat.c b/src/port/win32stat.c
index 4351aa4d08..2f9e111627 100644
--- a/src/port/win32stat.c
+++ b/src/port/win32stat.c
@@ -170,6 +170,7 @@ _pgstat64(const char *name, struct stat *buf)
SECURITY_ATTRIBUTES sa;
HANDLE hFile;
int ret;
+ int loops = 0;
#if _WIN32_WINNT < 0x0600
IO_STATUS_BLOCK ioStatus;
FILE_STANDARD_INFORMATION standardInfo;
@@ -182,31 +183,63 @@ _pgstat64(const char *name, struct stat *buf)
errno = EINVAL;
return -1;
}
-
/* fast not-exists check */
if (GetFileAttributes(name) == INVALID_FILE_ATTRIBUTES)
{
- _dosmaperr(GetLastError());
- return -1;
+ DWORD err = GetLastError();
+
+ if (err != ERROR_ACCESS_DENIED) {
+ _dosmaperr(err);
+ return -1;
+ }
}
/* get a file handle as lightweight as we can */
sa.nLength = sizeof(SECURITY_ATTRIBUTES);
sa.bInheritHandle = TRUE;
sa.lpSecurityDescriptor = NULL;
- hFile = CreateFile(name,
- GENERIC_READ,
- (FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE),
- &sa,
- OPEN_EXISTING,
- (FILE_FLAG_NO_BUFFERING | FILE_FLAG_BACKUP_SEMANTICS |
- FILE_FLAG_OVERLAPPED),
- NULL);
- if (hFile == INVALID_HANDLE_VALUE)
+ while ((hFile = CreateFile(name,
+ GENERIC_READ,
+ (FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE),
+ &sa,
+ OPEN_EXISTING,
+ (FILE_FLAG_NO_BUFFERING | FILE_FLAG_BACKUP_SEMANTICS |
+ FILE_FLAG_OVERLAPPED),
+ NULL)) == INVALID_HANDLE_VALUE)
{
DWORD err = GetLastError();
- CloseHandle(hFile);
+ /*
+ * 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)
+ {
+ struct microsoft_native_stat st;
+
+ if (microsoft_native_stat(name, &st) != 0)
+ {
+ pg_usleep(100000);
+ loops++;
+ continue;
+ }
+ }
+ }
+
_dosmaperr(err);
return -1;
}
