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
diff --git a/src/include/port/win32_port.h b/src/include/port/win32_port.h index 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.c index 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.c index 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);