From 033e4898a076e65162be3dcd60badaa40ba8f736 Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Mon, 3 Oct 2022 09:15:02 +1300 Subject: [PATCH 09/10] Fix unlink() for STATUS_DELETE_PENDING on Windows. Commit c5cb8f3b didn't handle STATUS_DELETE_PENDING correctly, and would report ENOENT immediately without waiting. If we called unlink(name) twice in a row and then rmdir(parent) while someone had an open handle, previously the second call to unlink() would block until that handle went away, or a 10 second timeout expired. This change resulted in a test occasionally failing on CI, which still has an OS without POSIX semantics for unlink. Restore. Diagnosed-by: Justin Pryzby Reviewed-by: Michael Paquier Discussion: https://postgr.es/m/20220920013122.GA31833%40telsasoft.com --- src/port/dirmod.c | 38 +++++++++++++++++++++++++++++++++++-- src/port/t/001_filesystem.c | 36 +++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/src/port/dirmod.c b/src/port/dirmod.c index f7fd4c15ad..f4abde97bf 100644 --- 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__) /* Externally visable only to allow testing. */ @@ -94,6 +98,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 @@ -101,6 +121,7 @@ pgrename(const char *from, const char *to) int pgunlink(const char *path) { + bool is_lnk; int loops = 0; struct stat st; @@ -125,9 +146,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 @@ -136,7 +170,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; diff --git a/src/port/t/001_filesystem.c b/src/port/t/001_filesystem.c index 501d7e1603..346978783a 100644 --- a/src/port/t/001_filesystem.c +++ b/src/port/t/001_filesystem.c @@ -472,6 +472,42 @@ filesystem_metadata_tests(void) PG_EXPECT_SYS(unlink(path) == 0, "Windows: can rename file after non-shared handle asynchronously closed"); #endif + /* + * Our Windows unlink() wrapper blocks in a retry loop if you try to + * unlink a file in STATUS_DELETE_PENDING (ie that has already been + * unlinked but is still open), until it times out with EACCES or reaches + * ENOENT. That may be useful for waiting for files to be asynchronously + * unlinked while performing a recursive unlink on + * !have_posix_unlink_semantics systems, so that rmdir(parent) works. + */ + make_path(path, "dir2"); + PG_EXPECT_SYS(mkdir(path, 0777) == 0); + make_path(path, "dir2/test-file"); + fd = open(path, O_CREAT | O_EXCL | O_RDWR | PG_BINARY, 0777); + PG_EXPECT_SYS(fd >= 0, "open dir2/test-file"); + PG_EXPECT_SYS(unlink(path) == 0, "unlink file while it's open, once"); +#ifdef WIN32 + pgwin32_dirmod_loops = 2; /* minimize looping to fail fast in testing */ +#endif + PG_EXPECT(unlink(path) == -1, "can't unlink again"); + if (have_posix_unlink_semantics) + { + PG_EXPECT_EQ(errno, ENOENT, "POSIX: we expect ENOENT"); + } + else + { + PG_EXPECT_EQ(errno, EACCES, "Windows non-POSIX: we expect EACCES (delete already pending)"); +#ifdef WIN32 + pgwin32_dirmod_loops = 1800; /* loop for up to 180s to make sure + * our 100ms callback is run */ + run_async_procedure_after_delay(close_fd, &fd, 100); /* close fd after 100ms */ +#endif + PG_EXPECT(unlink(path) == -1, "Windows non-POSIX: trying again fails"); + PG_EXPECT_EQ(errno, ENOENT, "Windows non-POSIX: ... but blocked until ENOENT was reached due to asynchronous close"); + } + make_path(path, "dir2"); + PG_EXPECT_SYS(rmdir(path) == 0, "now we can remove the directory"); + /* Tests for rename(). */ make_path(path, "name1.txt"); -- 2.37.3