From 54e72b66ed1a55c2fa558bc60d534bdc61dbb62f Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Tue, 31 Jan 2023 13:07:44 +1300 Subject: [PATCH] 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 --- src/common/rmtree.c | 122 +++++++++++++++++++++++--------------------- 1 file changed, 65 insertions(+), 57 deletions(-) diff --git a/src/common/rmtree.c b/src/common/rmtree.c index d445e21ba2..a3e536149b 100644 --- a/src/common/rmtree.c +++ b/src/common/rmtree.c @@ -20,13 +20,21 @@ #include #include +#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; + 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); - /* - * we copy all the names out of the directory before we start modifying - * it. - */ - filenames = pgfnames(path); - - if (filenames == NULL) - return false; - - /* now we have the names we can start removing things */ - for (filename = filenames; *filename; filename++) + dir = OPENDIR(path); + if (dir == NULL) { - snprintf(pathbuf, MAXPGPATH, "%s/%s", path, *filename); + pg_log_warning("could not open directory \"%s\": %m", path); + return false; + } - /* - * 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; - } + while (errno = 0, (de = readdir(dir))) + { + if (strcmp(de->d_name, ".") == 0 || + strcmp(de->d_name, "..") == 0) continue; - } + snprintf(pathbuf, sizeof(pathbuf), "%s/%s", path, de->d_name); + switch (get_dirent_type(pathbuf, de, false, LOG_LEVEL)) + { + case PGFILETYPE_ERROR: + /* already logged, press on */ + break; + case PGFILETYPE_DIR: - if (S_ISDIR(statbuf.st_mode)) - { - /* call ourselves recursively for a directory */ - if (!rmtree(pathbuf, true)) - { - /* we already reported the error */ - result = false; - } - } - else - { - if (unlink(pathbuf) != 0) - { - if (errno != ENOENT) + /* + * 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) { - pg_log_warning("could not remove file or directory \"%s\": %m", - pathbuf); + 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 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; }