From 5990d14bb4f9a0ca83fd1b6c7c6e0a6a23786a0d Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Sun, 5 Sep 2021 17:07:44 +1200 Subject: [PATCH] Fix Windows basebackup by renaming before unlinking. Rename files before unlinking on Windows. This solves a problem where basebackup could fail because other backends have a file handle to a deleted file, so stat() and open() fail with ERROR_ACCESS_DENIED. Suggested by Kyotaro Horiguchi . XXX This is only a rough sketch to try the idea out! Discussion: https://postgr.es/m/CA%2BhUKGJz_pZTF9mckn6XgSv69%2BjGwdgLkxZ6b3NWGLBCVjqUZA%40mail.gmail.com --- src/backend/replication/basebackup.c | 7 +++++ src/backend/storage/file/fd.c | 38 +++++++++++++++++++++++++--- src/backend/storage/smgr/md.c | 6 ++--- src/include/storage/fd.h | 1 + 4 files changed, 46 insertions(+), 6 deletions(-) diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c index e71d7406ed..7961a7041b 100644 --- a/src/backend/replication/basebackup.c +++ b/src/backend/replication/basebackup.c @@ -19,6 +19,7 @@ #include "access/xlog_internal.h" /* for pg_start/stop_backup */ #include "catalog/pg_type.h" #include "common/file_perm.h" +#include "common/string.h" #include "commands/progress.h" #include "lib/stringinfo.h" #include "libpq/libpq.h" @@ -1268,6 +1269,12 @@ sendDir(const char *path, int basepathlen, bool sizeonly, List *tablespaces, strlen(PG_TEMP_FILE_PREFIX)) == 0) continue; +#ifdef WIN32 + /* Skip unlinked files */ + if (pg_str_endswith(de->d_name, ".unlinked")) + continue; +#endif + /* * Check if the postmaster has signaled us to exit, and abort with an * error in that case. The error handler further up will call diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index 501652654e..43f24d4ceb 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -72,6 +72,7 @@ #include "postgres.h" +#include #include #include #include @@ -683,6 +684,35 @@ pg_truncate(const char *path, off_t length) #endif } +/* + * Unlink a file. On Windows, rename to a temporary filename before unlinking + * so that "path" is available for new files immediately even if other backends + * hold descriptors to the one we unlink. + */ +int +pg_unlink(const char *path) +{ +#ifdef WIN32 + for (;;) + { + char tmp_path[MAXPGPATH]; + + snprintf(tmp_path, sizeof(tmp_path), "%s.%ld.unlinked", path, random()); + if (!MoveFileEx(path, tmp_path, 0)) + { + if (GetLastError() == ERROR_FILE_EXISTS) + continue; /* try a new random number */ + _dosmaperr(GetLastError()); + return -1; + } + + return unlink(tmp_path); + } +#else + return unlink(path); +#endif +} + /* * fsync_fname -- fsync a file or directory, handling errors properly * @@ -3269,8 +3299,9 @@ CleanupTempFiles(bool isCommit, bool isProcExit) * postmaster session * * This should be called during postmaster startup. It will forcibly - * remove any leftover files created by OpenTemporaryFile and any leftover - * temporary relation files created by mdcreate. + * remove any leftover files created by OpenTemporaryFile, any leftover + * temporary relation files created by mdcreate, and anything left + * behind by pg_unlink() on crash. * * During post-backend-crash restart cycle, this routine is called when * remove_temp_files_after_crash GUC is enabled. Multiple crashes while @@ -3448,7 +3479,8 @@ RemovePgTempRelationFilesInDbspace(const char *dbspacedirname) while ((de = ReadDirExtended(dbspace_dir, dbspacedirname, LOG)) != NULL) { - if (!looks_like_temp_rel_name(de->d_name)) + if (!looks_like_temp_rel_name(de->d_name) && + !pg_str_endswith(de->d_name, ".unlinked")) continue; snprintf(rm_path, sizeof(rm_path), "%s/%s", diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c index 8592d47e95..d519855d59 100644 --- a/src/backend/storage/smgr/md.c +++ b/src/backend/storage/smgr/md.c @@ -374,7 +374,7 @@ mdunlinkfork(RelFileNodeBackend rnode, ForkNumber forkNum, bool isRedo) /* Next unlink the file, unless it was already found to be missing */ if (ret == 0 || errno != ENOENT) { - ret = unlink(path); + ret = pg_unlink(path); if (ret < 0 && errno != ENOENT) ereport(WARNING, (errcode_for_file_access(), @@ -422,7 +422,7 @@ mdunlinkfork(RelFileNodeBackend rnode, ForkNumber forkNum, bool isRedo) register_forget_request(rnode, forkNum, segno); } - if (unlink(segpath) < 0) + if (pg_unlink(segpath) < 0) { /* ENOENT is expected after the last segment... */ if (errno != ENOENT) @@ -1753,7 +1753,7 @@ mdunlinkfiletag(const FileTag *ftag, char *path) pfree(p); /* Try to unlink the file. */ - return unlink(path); + return pg_unlink(path); } /* diff --git a/src/include/storage/fd.h b/src/include/storage/fd.h index 329c7eba9a..be38f6a979 100644 --- a/src/include/storage/fd.h +++ b/src/include/storage/fd.h @@ -189,6 +189,7 @@ extern ssize_t pg_pwritev_with_retry(int fd, int iovcnt, off_t offset); extern int pg_truncate(const char *path, off_t length); +extern int pg_unlink(const char *path); extern void fsync_fname(const char *fname, bool isdir); extern int fsync_fname_ext(const char *fname, bool isdir, bool ignore_perm, int elevel); extern int durable_rename(const char *oldfile, const char *newfile, int loglevel); -- 2.30.2