Subject: Re: More time spending with "delete pending"
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>, Tom Lane
 <tgl@sss.pgh.pa.us>, Justin Pryzby <pryzby@telsasoft.com>
References: <20201115011112.GW30691@telsasoft.com>
 <979f4ee6-9960-e37f-f3ee-f458e87777b0@gmail.com>
 <995713.1605725597@sss.pgh.pa.us> <1001775.1605731995@sss.pgh.pa.us>
 <3366bfde-183f-60be-62ab-8ae81b946bac@gmail.com>
 <1006364.1605738508@sss.pgh.pa.us>
 <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>
From: Alexander Lakhin <exclusion@gmail.com>
Message-ID: <d030cc87-641c-c2f0-b7a9-4d6669ea91d7@gmail.com>
Date: Thu, 8 Jul 2021 23:00:00 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.1
MIME-Version: 1.0
In-Reply-To: <YOatgGjMgnEFsnGL@paquier.xyz>
Content-Type: multipart/mixed;
 boundary="------------1650A2FB237BB018CCEACB58"
Content-Language: en-US
Archived-At: 
 <https://www.postgresql.org/message-id/d030cc87-641c-c2f0-b7a9-4d6669ea91d7%40gmail.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------1650A2FB237BB018CCEACB58
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit

08.07.2021 10:47, Michael Paquier wrote:
> On Thu, Jul 08, 2021 at 07:00:01AM +0300, Alexander Lakhin wrote:
>> As Tom Lane noted above, the code added with bed90759f is dubious
>> (_NtQueryInformationFile() can not be used to handle the "delete
>> pending" state as CreateFile() returns INVALID_HANDLE_VALUE in this case.)
>> Probably that change should be reverted. Should I do it along with the
>> proposed fix?
> Ah, I see.  I have managed to miss your point.  If
> _NtQueryInformationFile() cannot be used, then we'd actually miss the
> contents for standardInfo and the pending deletion.  If you could send
> everything you have, that would be helpful!  I'd like to test that
> stuff by myself, with all the contents discussed at disposal for a
> proper evaluation.
> --
Beside the aforementioned test I can only propose the extended patch,
that incorporates the undo of the changes brought by bed90759f.
With this patch that test is passed.

Best regards,
Alexander

--------------1650A2FB237BB018CCEACB58
Content-Type: text/x-patch; charset=UTF-8;
 name="fix_pg_ls_dir_v2.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="fix_pg_ls_dir_v2.patch"

diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c
index c436d9318b..a8c22a81d0 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 2ad8ee1359..6028ad809e 100644
--- a/src/port/win32stat.c
+++ b/src/port/win32stat.c
@@ -18,53 +18,6 @@
 #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 */
-
-
 /*
  * Convert a FILETIME struct into a 64 bit time_t.
  */
@@ -170,110 +123,73 @@ _pgstat64(const char *name, struct stat *buf)
 	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;
+		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);
-		_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)
-		{
-			DWORD		err = GetLastError();
-
-			CloseHandle(hFile);
-			_dosmaperr(err);
-			return -1;
-		}
-
-		_NtQueryInformationFile = (PFN_NTQUERYINFORMATIONFILE) (pg_funcptr_t)
-			GetProcAddress(ntdll, "NtQueryInformationFile");
-		if (_NtQueryInformationFile == NULL)
+		/*
+		 * 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)
 		{
-			DWORD		err = GetLastError();
-
-			CloseHandle(hFile);
-			_dosmaperr(err);
-			return -1;
+			if (loops < 10)
+			{
+				struct microsoft_native_stat st;
+
+				if (microsoft_native_stat(name, &st) != 0)
+				{
+					pg_usleep(100000);
+					loops++;
+					continue;
+				}
+			}
 		}
-	}
-
-	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)))
-	{
-		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.
-		 */
-		CloseHandle(hFile);
-		errno = ENOENT;
-		return -1;
-	}
 
 	/* At last we can invoke fileinfo_to_stat */
 	ret = fileinfo_to_stat(hFile, buf);

--------------1650A2FB237BB018CCEACB58--