On Wed, Dec 7, 2022 at 7:15 AM Andres Freund <and...@anarazel.de> wrote: > On 2022-11-08 01:16:09 +1300, Thomas Munro wrote: > > So [1] on its own didn't fix this. My next guess is that the attached > > might help.
> What is our plan here? This afaict is the most common "false positive" for > cfbot in the last weeks. That branch hasn't failed on cfbot[1], except once due to one of the other known flapping races we have to fix. Which doesn't prove anything, of course, but it is encouraging. I wish we knew why the test does this, though.... Here's a better version that works harder to avoid opening more than one fd at a time (like the pgfnames()-based code it replaces), and also uses fd.c facilities in the backend version (unlike pgfnames(), which looks like it could leak a descriptor if palloc() threw, and also doesn't know how to handle file descriptor pressure). [1] https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/41/4011
From 3c9ed22ee207f2a5c008a971f8b19561d2e0ccfa Mon Sep 17 00:00:00 2001 From: Thomas Munro <tmu...@postgresql.org> Date: Thu, 5 Jan 2023 14:15:37 +1300 Subject: [PATCH v2] Refactor rmtree() to use get_dirent_type(). Switch to get_dirent_type() instead of lstat() while traversing a directory graph, to see if that fixes intermittent ENOTEMPTY failures while cleaning up after the pg_upgrade tests, on our Windows CI build. Our CI image uses Windows Server 2019, a version known not to have POSIX unlink semantics enabled by default yet, unlike typical Windows 10 and 11 systems. The theory is that the offending directory entries must be STATUS_DELETE_PENDING. With this change, rmtree() should not skip them, and try to unlink again. Our unlink() wrapper should either wait a short time for them to go away (because other processes close the handle) or log a message to tell us the path of the problem file if not, so we can dig further. Discussion: https://postgre.es/m/20220919213217.ptqfdlcc5idk5xup%40awork3.anarazel.de --- src/common/rmtree.c | 120 +++++++++++++++++++++++--------------------- 1 file changed, 64 insertions(+), 56 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 <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; } -- 2.35.1
