From: Alexander Lakhin <exclusion@gmail.com>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: More time spending with "delete pending"
Date: Sun, 15 Nov 2020 08:00:00 +0300
Message-ID: <979f4ee6-9960-e37f-f3ee-f458e87777b0@gmail.com> (raw)
In-Reply-To: <20201115011112.GW30691@telsasoft.com>
References: <c3427edf-d7c0-ff57-90f6-b5de3bb62709@gmail.com>
<20201115011112.GW30691@telsasoft.com>
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
Attachments:fix_pg_ls_dir.patch (text/x-patch, 3.2K) downloaddiff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.cindex 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.cindex 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;
}
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-hackers@postgresql.org
Cc: exclusion@gmail.com, pryzby@telsasoft.com
Subject: Re: More time spending with "delete pending"
In-Reply-To: <979f4ee6-9960-e37f-f3ee-f458e87777b0@gmail.com>
* 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