Fix unlink() for STATUS_DELETE_PENDING on Windows.

Commit f357233c 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 <[email protected]>
Reviewed-by: Michael Paquier <[email protected]>
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
index d83316d6a2ae4724317d79c7c2fbfd45a702c30d..3f45f6a65501604f425b177e269b014fdfce4e34 100644 (file)
--- 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,6 +95,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
@@ -98,6 +118,7 @@ pgrename(const char *from, const char *to)
 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;