Fix unlink() for STATUS_DELETE_PENDING on Windows.

macdice · macdice · commit e109e43921d2 · 2022-10-25T16:26:58.000+13:00
Commit f357233 assumed that it was OK to return ENOENT directly if lstat() failed that way. If we got STATUS_DELETE_PENDING while trying to unlink a file that we had already unlinked successfully once before but someone else still had open (on a kernel version that has "pending" unlinks by default), then we would no longer reach the retry loop in pgunlink(). That loop claims to be only for handling sharing violations (a different phenomenon), but the errno is the same. Restore that behavior with an explicit check, to see if it fixes the occasional 'directory not empty' failures seen in the pg_upgrade tests on CI. Further improvements are possible with proposed upgrades to modern Windows APIs that would replace this convoluted code. Reported-by: Justin Pryzby <pryzby@telsasoft.com> Reviewed-by: Michael Paquier <michael@paquier.xyz> Discussion: https://postgr.es/m/20220920013122.GA31833%40telsasoft.com Discussion: https://postgr.es/m/CA%2BhUKG%2BajSQ_8eu2AogTncOnZ5me2D-Cn66iN_-wZnRjLN%2Bicg%40mail.gmail.com
diff --git 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__)
 
 /*
@@ -91,13 +95,30 @@ 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
  */
 int
 pgunlink(const char *path)
 {
+	bool		is_lnk;
 	int			loops = 0;
 	struct stat st;
 
@@ -122,9 +143,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
@@ -133,7 +167,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;
