From: Alexander Lakhin <exclusion@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Subject: Re: More time spending with "delete pending"
Date: Tue, 13 Jul 2021 21:00:00 +0300
Message-ID: <62c17068-a608-800f-295e-9d2cca5decf7@gmail.com> (raw)
In-Reply-To: <YOvYnj2pB/EB2l8A@paquier.xyz>
References: <20729794-e2bf-bd7a-f01b-d132f89fd0d7@gmail.com>
<fcc8aea0-14d2-9fab-1a4e-37fd1925c49b@gmail.com>
<YOQVX1f9/2s1ct6W@paquier.xyz>
<78fa6a17-8e71-7c99-62bd-8a11a47fe894@gmail.com>
<YOatgGjMgnEFsnGL@paquier.xyz>
<d030cc87-641c-c2f0-b7a9-4d6669ea91d7@gmail.com>
<YOfkMRNtMjyDLnLL@paquier.xyz>
<2aa7ce7a-d73a-fa36-4fb5-3affe1860c58@gmail.com>
<YOu/9Zc/LuL1Lscu@paquier.xyz>
<YOvOlfRrIO0yGtgw@paquier.xyz>
<YOvYnj2pB/EB2l8A@paquier.xyz>
Hello Michael,
12.07.2021 08:52, Michael Paquier wrote:
> On Mon, Jul 12, 2021 at 02:09:41PM +0900, Michael Paquier wrote:
>> And fairywren, that uses MinGW, is unhappy:
>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2021-07-12%2004%3A47%3A13
>> Looking at it now.
> I am not sure what to do here to cool down MinGW, so the patch has
> been reverted for now:
> - Plain stat() is not enough to do a proper detection of the files
> pending for deletion on MSVC, so reverting only the part with
> microsoft_native_stat() is not going to help.
> - We are going to need an evaluation of the stat() implementation in
> MinGW and check if is it possible to rely on it more. If yes, we
> could get away with more tweaks based on __MINGW32__/__MINGW64__.
I've managed to adapt open/win32stat for MinGW by preventing stat
overriding at the implementation level. Thus, the code that calls stat()
will use the overridden struct/function(s), but inside of
open/win32_stat we could access original stat and reference to
__pgstat64 where we want to use our.
Tested on MSVC, MinGW64 and MinGW32 — the fixed code compiles everywhere.
But when using perl from msys (not ActivePerl) while testing I've got
the same test failure due to another error condition:
In this case CreateFile() fails with ERROR_SHARING_VIOLATION (not
ERROR_ACCESS_DENIED) and it leads to the same "Permission denied" error.
This condition is handled in the pgwin32_open() but not in _pgstat64() yet.
I think I should develop another test for MSVC environment that will
demonstrate a such failure too.
But as it's not related to the DELETE_PENDING state, I wonder whether
this should be fixed in a separate patch.
Best regards,
Alexander
Attachments:ghffjapimnefdgac.png (image/png, 35.1K) downloadview imagewin32-stat-deletion-v2.patch (text/x-patch, 8.6K) downloaddiff --git a/src/include/port/win32_port.h b/src/include/port/win32_port.hindex 05c5a53442..d72950880a 100644--- a/src/include/port/win32_port.h+++ b/src/include/port/win32_port.h@@ -53,11 +53,13 @@
#undef near
/* needed before sys/stat hacking below: */
+#if !defined(NO_STAT_OVERRIDE)
#define fstat microsoft_native_fstat
#define stat microsoft_native_stat
#include <sys/stat.h>
#undef fstat
#undef stat
+#endif
/* Must be here to avoid conflicting with prototype in windows.h */
#define mkdir(a,b) mkdir(a)
@@ -253,7 +255,7 @@ typedef int pid_t;
* The struct stat is 32 bit in MSVC, so we redefine it as a copy of
* struct __stat64. This also fixes the struct size for MINGW builds.
*/
-struct stat /* This should match struct __stat64 */+struct _pgstat64 /* This should match struct __stat64 */
{
_dev_t st_dev;
_ino_t st_ino;
@@ -268,12 +270,14 @@ struct stat /* This should match struct __stat64 */
__time64_t st_ctime;
};
-extern int _pgfstat64(int fileno, struct stat *buf);-extern int _pgstat64(const char *name, struct stat *buf);+extern int _pgfstat64(int fileno, struct _pgstat64 *buf);+extern int _pgstat64(const char *name, struct _pgstat64 *buf);+#if !defined(NO_STAT_OVERRIDE)
#define fstat(fileno, sb) _pgfstat64(fileno, sb)
-#define stat(path, sb) _pgstat64(path, sb)+#define stat _pgstat64
#define lstat(path, sb) _pgstat64(path, sb)
+#endif
/* These macros are not provided by older MinGW, nor by MSVC */
#ifndef S_IRUSR
diff --git a/src/port/open.c b/src/port/open.cindex 14c6debba9..09144351a4 100644--- a/src/port/open.c+++ b/src/port/open.c@@ -13,6 +13,7 @@
#ifdef WIN32
+#define NO_STAT_OVERRIDE
#ifndef FRONTEND
#include "postgres.h"
#else
diff --git a/src/port/win32stat.c b/src/port/win32stat.cindex 2ad8ee1359..1c9c01cc98 100644--- a/src/port/win32stat.c+++ b/src/port/win32stat.c@@ -15,54 +15,10 @@
#ifdef WIN32
+#define NO_STAT_OVERRIDE
#include "c.h"
#include <windows.h>
--/*- * In order to support MinGW and MSVC2013 we use NtQueryInformationFile as an- * alternative for GetFileInformationByHandleEx. It is loaded from the ntdll- * library.- */-#if _WIN32_WINNT < 0x0600-#include <winternl.h>--#if !defined(__MINGW32__) && !defined(__MINGW64__)-/* MinGW includes this in <winternl.h>, but it is missing in MSVC */-typedef struct _FILE_STANDARD_INFORMATION-{- LARGE_INTEGER AllocationSize;- LARGE_INTEGER EndOfFile;- ULONG NumberOfLinks;- BOOLEAN DeletePending;- BOOLEAN Directory;-} FILE_STANDARD_INFORMATION;-#define FileStandardInformation 5-#endif /* !defined(__MINGW32__) &&- * !defined(__MINGW64__) */--typedef NTSTATUS (NTAPI * PFN_NTQUERYINFORMATIONFILE)- (IN HANDLE FileHandle,- OUT PIO_STATUS_BLOCK IoStatusBlock,- OUT PVOID FileInformation,- IN ULONG Length,- IN FILE_INFORMATION_CLASS FileInformationClass);--static PFN_NTQUERYINFORMATIONFILE _NtQueryInformationFile = NULL;--static HMODULE ntdll = NULL;--/*- * Load DLL file just once regardless of how many functions we load/call in it.- */-static void-LoadNtdll(void)-{- if (ntdll != NULL)- return;- ntdll = LoadLibraryEx("ntdll.dll", NULL, 0);-}--#endif /* _WIN32_WINNT < 0x0600 */+#include <sys/stat.h>
/*
@@ -112,7 +68,7 @@ fileattr_to_unixmode(int attr)
* Convert WIN32 file information (from a HANDLE) to a struct stat.
*/
static int
-fileinfo_to_stat(HANDLE hFile, struct stat *buf)+fileinfo_to_stat(HANDLE hFile, struct _pgstat64 *buf)
{
BY_HANDLE_FILE_INFORMATION fiData;
@@ -159,119 +115,83 @@ fileinfo_to_stat(HANDLE hFile, struct stat *buf)
* This currently also implements lstat(), though perhaps that should change.
*/
int
-_pgstat64(const char *name, struct stat *buf)+_pgstat64(const char *name, struct _pgstat64 *buf)
{
/*
* We must use a handle so lstat() returns the information of the target
- * file. To have a reliable test for ERROR_DELETE_PENDING, we use- * NtQueryInformationFile from Windows 2000 or- * GetFileInformationByHandleEx from Server 2008 / Vista.+ * file. To have a reliable test for ERROR_DELETE_PENDING, this uses a+ * method similar to open() with a loop using stat() and some waits when+ * facing ERROR_ACCESS_DENIED.
*/
SECURITY_ATTRIBUTES sa;
HANDLE hFile;
int ret;
-#if _WIN32_WINNT < 0x0600- IO_STATUS_BLOCK ioStatus;- FILE_STANDARD_INFORMATION standardInfo;-#else- FILE_STANDARD_INFO standardInfo;-#endif+ int loops = 0;
if (name == NULL || buf == NULL)
{
errno = EINVAL;
return -1;
}
-
/* fast not-exists check */
if (GetFileAttributes(name) == INVALID_FILE_ATTRIBUTES)
- {- _dosmaperr(GetLastError());- 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)
{
DWORD err = GetLastError();
- CloseHandle(hFile);- _dosmaperr(err);- return -1;- }-- memset(&standardInfo, 0, sizeof(standardInfo));--#if _WIN32_WINNT < 0x0600- if (_NtQueryInformationFile == NULL)- {- /* First time through: load ntdll.dll and find NtQueryInformationFile */- LoadNtdll();- if (ntdll == NULL)+ if (err != ERROR_ACCESS_DENIED)
{
- DWORD err = GetLastError();-- CloseHandle(hFile);- _dosmaperr(err);- return -1;- }-- _NtQueryInformationFile = (PFN_NTQUERYINFORMATIONFILE) (pg_funcptr_t)- GetProcAddress(ntdll, "NtQueryInformationFile");- if (_NtQueryInformationFile == NULL)- {- DWORD err = GetLastError();-- CloseHandle(hFile);
_dosmaperr(err);
return -1;
}
}
- if (!NT_SUCCESS(_NtQueryInformationFile(hFile, &ioStatus, &standardInfo,- sizeof(standardInfo),- FileStandardInformation)))- {- DWORD err = GetLastError();-- CloseHandle(hFile);- _dosmaperr(err);- return -1;- }-#else- if (!GetFileInformationByHandleEx(hFile, FileStandardInfo, &standardInfo,- sizeof(standardInfo)))+ /* get a file handle as lightweight as we can */+ sa.nLength = sizeof(SECURITY_ATTRIBUTES);+ sa.bInheritHandle = TRUE;+ sa.lpSecurityDescriptor = NULL;+ 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);- _dosmaperr(err);- return -1;- }-#endif /* _WIN32_WINNT < 0x0600 */-- if (standardInfo.DeletePending)- {
/*
- * File has been deleted, but is not gone from the filesystem yet.- * This can happen when some process with FILE_SHARE_DELETE has it- * open, and it will be fully removed once that handle is closed.- * Meanwhile, we can't open it, so indicate that the file just doesn't- * exist.+ * 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.
*/
- CloseHandle(hFile);- errno = ENOENT;+ if (err == ERROR_ACCESS_DENIED)+ {+ if (loops < 10)+ {+ struct stat st;++ if (stat(name, &st) != 0)+ {+ pg_usleep(100000);+ loops++;+ continue;+ }+ }+ }++ _dosmaperr(err);
return -1;
}
@@ -286,7 +206,7 @@ _pgstat64(const char *name, struct stat *buf)
* Windows implementation of fstat().
*/
int
-_pgfstat64(int fileno, struct stat *buf)+_pgfstat64(int fileno, struct _pgstat64 *buf)
{
HANDLE hFile = (HANDLE) _get_osfhandle(fileno);
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, michael@paquier.xyz, tgl@sss.pgh.pa.us, pryzby@telsasoft.com
Subject: Re: More time spending with "delete pending"
In-Reply-To: <62c17068-a608-800f-295e-9d2cca5decf7@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