Refactor rmtree() to use get_dirent_type().

Switch to get_dirent_type() instead of lstat() while traversing a
directory tree, to see if that fixes the intermittent ENOTEMPTY failures
seen in recent pg_upgrade tests, on Windows CI.  While refactoring, also
use AllocateDir() instead of opendir() in the backend, which knows how
to handle descriptor pressure.

Our CI system currently uses Windows Server 2019, a version known not to
have POSIX unlink semantics enabled by default yet, unlike typical
Windows 10 and 11 systems.  That might explain why we see this flapping
on CI but (apparently) not in the build farm, though the frequency is
quite low.

The theory is that some directory entry must be in state
STATUS_DELETE_PENDING, which lstat() would report as ENOENT, though
unfortunately we don't know exactly why yet.  With this change, rmtree()
will not skip them, and try to unlink (again).  Our unlink() wrapper
should either wait a short time for them to go away when some other
process closes the handle, or log a message to tell us the path of the
problem file if not, so we can dig further.

Discussion: https://postgr.es/m/20220919213217.ptqfdlcc5idk5xup%40awork3.anarazel.de

diff --git a/src/common/rmtree.c b/src/common/rmtree.c
index d445e21ba230b2c14f3d72ed1022d4d72c17fc96..a3e536149bed8f240b3b52187236f7e3ea33b6a1 100644 (file)
--- a/src/common/rmtree.c
+++ b/src/common/rmtree.c
@@ -20,13 +20,21 @@
 #include <unistd.h>
 #include <sys/stat.h>
 
+#include "common/file_utils.h"
+
 #ifndef FRONTEND
+#include "storage/fd.h"
 #define pg_log_warning(...) elog(WARNING, __VA_ARGS__)
+#define LOG_LEVEL WARNING
+#define OPENDIR(x) AllocateDir(x)
+#define CLOSEDIR(x) FreeDir(x)
 #else
 #include "common/logging.h"
+#define LOG_LEVEL PG_LOG_WARNING
+#define OPENDIR(x) opendir(x)
+#define CLOSEDIR(x) closedir(x)
 #endif
 
-
 /*
  * rmtree
  *
@@ -41,82 +49,82 @@
 bool
 rmtree(const char *path, bool rmtopdir)
 {
-   bool        result = true;
    char        pathbuf[MAXPGPATH];
-   char      **filenames;
-   char      **filename;
-   struct stat statbuf;
-
-   /*
-    * we copy all the names out of the directory before we start modifying
-    * it.
-    */
-   filenames = pgfnames(path);
+   DIR        *dir;
+   struct dirent *de;
+   bool        result = true;
+   size_t      dirnames_size = 0;
+   size_t      dirnames_capacity = 8;
+   char      **dirnames = palloc(sizeof(char *) * dirnames_capacity);
 
-   if (filenames == NULL)
+   dir = OPENDIR(path);
+   if (dir == NULL)
+   {
+       pg_log_warning("could not open directory \"%s\": %m", path);
        return false;
+   }
 
-   /* now we have the names we can start removing things */
-   for (filename = filenames; *filename; filename++)
+   while (errno = 0, (de = readdir(dir)))
    {
-       snprintf(pathbuf, MAXPGPATH, "%s/%s", path, *filename);
-
-       /*
-        * It's ok if the file is not there anymore; we were just about to
-        * delete it anyway.
-        *
-        * This is not an academic possibility. One scenario where this
-        * happens is when bgwriter has a pending unlink request for a file in
-        * a database that's being dropped. In dropdb(), we call
-        * ForgetDatabaseSyncRequests() to flush out any such pending unlink
-        * requests, but because that's asynchronous, it's not guaranteed that
-        * the bgwriter receives the message in time.
-        */
-       if (lstat(pathbuf, &statbuf) != 0)
-       {
-           if (errno != ENOENT)
-           {
-               pg_log_warning("could not stat file or directory \"%s\": %m",
-                              pathbuf);
-               result = false;
-           }
+       if (strcmp(de->d_name, ".") == 0 ||
+           strcmp(de->d_name, "..") == 0)
            continue;
-       }
-
-       if (S_ISDIR(statbuf.st_mode))
-       {
-           /* call ourselves recursively for a directory */
-           if (!rmtree(pathbuf, true))
-           {
-               /* we already reported the error */
-               result = false;
-           }
-       }
-       else
+       snprintf(pathbuf, sizeof(pathbuf), "%s/%s", path, de->d_name);
+       switch (get_dirent_type(pathbuf, de, false, LOG_LEVEL))
        {
-           if (unlink(pathbuf) != 0)
-           {
-               if (errno != ENOENT)
+           case PGFILETYPE_ERROR:
+               /* already logged, press on */
+               break;
+           case PGFILETYPE_DIR:
+
+               /*
+                * Defer recursion until after we've closed this directory, to
+                * avoid using more than one file descriptor at a time.
+                */
+               if (dirnames_size == dirnames_capacity)
+               {
+                   dirnames = repalloc(dirnames,
+                                       sizeof(char *) * dirnames_capacity * 2);
+                   dirnames_capacity *= 2;
+               }
+               dirnames[dirnames_size++] = pstrdup(pathbuf);
+               break;
+           default:
+               if (unlink(pathbuf) != 0 && errno != ENOENT)
                {
-                   pg_log_warning("could not remove file or directory \"%s\": %m",
-                                  pathbuf);
+                   pg_log_warning("could not unlink file \"%s\": %m", pathbuf);
                    result = false;
                }
-           }
+               break;
        }
    }
 
+   if (errno != 0)
+   {
+       pg_log_warning("could not read directory \"%s\": %m", path);
+       result = false;
+   }
+
+   CLOSEDIR(dir);
+
+   /* Now recurse into the subdirectories we found. */
+   for (size_t i = 0; i < dirnames_size; ++i)
+   {
+       if (!rmtree(dirnames[i], true))
+           result = false;
+       pfree(dirnames[i]);
+   }
+
    if (rmtopdir)
    {
        if (rmdir(path) != 0)
        {
-           pg_log_warning("could not remove file or directory \"%s\": %m",
-                          path);
+           pg_log_warning("could not remove directory \"%s\": %m", path);
            result = false;
        }
    }
 
-   pgfnames_cleanup(filenames);
+   pfree(dirnames);
 
    return result;
 }