v2-0009-Fix-unlink-for-STATUS_DELETE_PENDING-on-Windows.patch

From 2913435d08ae49019e1338fd0563fedae63069cb Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Mon, 3 Oct 2022 09:15:02 +1300
Subject: [PATCH v2 09/10] Fix unlink() for STATUS_DELETE_PENDING on Windows.

Commit c5cb8f3b didn't handle STATUS_DELETE_PENDING correctly, and would
report ENOENT immediately without waiting.  If we called unlink(name)
twice in a row and then rmdir(parent) while someone had an open handle,
previously the second call to unlink() would block until that handle
went away, or a 10 second timeout expired.  This change resulted in a
test occasionally failing on CI, which still has an OS without POSIX
semantics for unlink.  Restore.

Diagnosed-by: Justin Pryzby <pryzby@telsasoft.com>
Reviewed-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/20220920013122.GA31833%40telsasoft.com
---
 src/port/dirmod.c           | 38 +++++++++++++++++++++++++++++++++++--
 src/port/t/001_filesystem.c | 36 +++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+), 2 deletions(-)

diff --git a/src/port/dirmod.c b/src/port/dirmod.c
index cf06878e1c..e377d46782 100644
--- a/src/port/dirmod.c
+++ b/src/port/dirmod.c
@@ -39,6 +39,10 @@
 #endif
 #endif
 
+#if defined(WIN32) && !defined(__CYGWIN__)
+#include "port/win32ntdll.h"
+#endif
+
 #if defined(WIN32) || defined(__CYGWIN__)
 
 /* Externally visable only to allow testing. */
@@ -94,6 +98,22 @@ pgrename(const char *from, const char *to)
 	return 0;
 }
 
+/*
+ * Check if _pglstat64()'s reason for failure was STATUS_DELETE_PENDING.
+ * This doesn't apply to Cygwin, which has its own lstat() that would report
+ * the case as EACCES.
+*/
+static bool
+lstat_error_was_status_delete_pending(void)
+{
+	if (errno != ENOENT)
+		return false;
+#if defined(WIN32) && !defined(__CYGWIN__)
+	if (pg_RtlGetLastNtStatus() == STATUS_DELETE_PENDING)
+		return true;
+#endif
+	return false;
+}
 
 /*
  *	pgunlink
@@ -101,6 +121,7 @@ pgrename(const char *from, const char *to)
 int
 pgunlink(const char *path)
 {
+	bool		is_lnk;
 	int			loops = 0;
 	struct stat st;
 
@@ -125,9 +146,22 @@ pgunlink(const char *path)
 	 * due to sharing violations, but that seems unlikely.  We could perhaps
 	 * prevent that by holding a file handle ourselves across the lstat() and
 	 * the retry loop, but that seems like over-engineering for now.
+	 *
+	 * In the special case of a STATUS_DELETE_PENDING error (file already
+	 * unlinked, but someone still has it open), we don't want to report ENOENT
+	 * to the caller immediately, because rmdir(parent) would probably fail.
+	 * We want to wait until the file truly goes away so that simple recursive
+	 * directory unlink algorithms work.
 	 */
 	if (lstat(path, &st) < 0)
-		return -1;
+	{
+		if (lstat_error_was_status_delete_pending())
+			is_lnk = false;
+		else
+			return -1;
+	}
+	else
+		is_lnk = S_ISLNK(st.st_mode);
 
 	/*
 	 * We need to loop because even though PostgreSQL uses flags that allow
@@ -136,7 +170,7 @@ pgunlink(const char *path)
 	 * someone else to close the file, as the caller might be holding locks
 	 * and blocking other backends.
 	 */
-	while ((S_ISLNK(st.st_mode) ? rmdir(path) : unlink(path)) < 0)
+	while ((is_lnk ? rmdir(path) : unlink(path)) < 0)
 	{
 		if (errno != EACCES)
 			return -1;
diff --git a/src/port/t/001_filesystem.c b/src/port/t/001_filesystem.c
index 35abeb772b..680e2353d7 100644
--- a/src/port/t/001_filesystem.c
+++ b/src/port/t/001_filesystem.c
@@ -476,6 +476,42 @@ filesystem_metadata_tests(void)
 	PG_EXPECT_SYS(unlink(path) == 0, "Windows: can rename file after non-shared handle asynchronously closed");
 #endif
 
+	/*
+	 * Our Windows unlink() wrapper blocks in a retry loop if you try to
+	 * unlink a file in STATUS_DELETE_PENDING (ie that has already been
+	 * unlinked but is still open), until it times out with EACCES or reaches
+	 * ENOENT.  That may be useful for waiting for files to be asynchronously
+	 * unlinked while performing a recursive unlink on
+	 * !have_posix_unlink_semantics systems, so that rmdir(parent) works.
+	 */
+	make_path(path, "dir2");
+	PG_EXPECT_SYS(mkdir(path, 0777) == 0);
+	make_path(path, "dir2/test-file");
+	fd = open(path, O_CREAT | O_EXCL | O_RDWR | PG_BINARY, 0777);
+	PG_EXPECT_SYS(fd >= 0, "open dir2/test-file");
+	PG_EXPECT_SYS(unlink(path) == 0, "unlink file while it's open, once");
+#ifdef WIN32
+	pgwin32_dirmod_loops = 2;	/* minimize looping to fail fast in testing */
+#endif
+	PG_EXPECT(unlink(path) == -1, "can't unlink again");
+	if (have_posix_unlink_semantics)
+	{
+		PG_EXPECT_EQ(errno, ENOENT, "POSIX: we expect ENOENT");
+	}
+	else
+	{
+		PG_EXPECT_EQ(errno, EACCES, "Windows non-POSIX: we expect EACCES (delete already pending)");
+#ifdef WIN32
+		pgwin32_dirmod_loops = 1800;	/* loop for up to 180s to make sure
+										 * our 100ms callback is run */
+		run_async_procedure_after_delay(close_fd, &fd, 100);	/* close fd after 100ms */
+#endif
+		PG_EXPECT(unlink(path) == -1, "Windows non-POSIX: trying again fails");
+		PG_EXPECT_EQ(errno, ENOENT, "Windows non-POSIX: ... but blocked until ENOENT was reached due to asynchronous close");
+	}
+	make_path(path, "dir2");
+	PG_EXPECT_SYS(rmdir(path) == 0, "now we can remove the directory");
+
 	/* Tests for rename(). */
 
 	make_path(path, "name1.txt");
-- 
2.35.1