Fix concurrency issues with WAL segment recycling on Windows

This commit is mostly a revert of aaa3aed, that switched the routine
doing the internal renaming of recycled WAL segments to use on Windows a
combination of CreateHardLinkA() plus unlink() instead of rename().  As
reported by several users of Postgres 13, this is causing concurrency
issues when manipulating WAL segments, mostly in the shape of the
following error:
LOG:  could not rename file "pg_wal/000000XX000000YY000000ZZ":
Permission denied

This moves back to a logic where a single rename() (well, pgrename() for
Windows) is used.  This issue has proved to be hard to hit when I tested
it, facing it only once with an archive_command that was not able to do
its work, so it is environment-sensitive.  The reporters of this issue
have been able to confirm that the situation improved once we switched
back to a single rename().  In order to check things, I have provided to
the reporters a patched build based on 13.2 with aaa3aed reverted, to
test if the error goes away, and an unpatched build of 13.2 to test if
the error still showed up (just to make sure that I did not mess up my
build process).

Extra thanks to Fujii Masao for pointing out what looked like the
culprit commit, and to all the reporters for taking the time to test
what I have sent them.

Reported-by: Andrus, Guy Burgess, Yaroslav Pashinsky, Thomas Trenz
Reviewed-by: Tom Lane, Andres Freund
Discussion: https://postgr.es/m/3861ff1e-0923-7838-e826-094cc9bef737@hot.ee
Discussion: https://postgr.es/m/16874-c3eecd319e36a2bf@postgresql.org
Discussion: https://postgr.es/m/095ccf8d-7f58-d928-427c-b17ace23cae6@burgess.co.nz
Discussion: https://postgr.es/m/16927-67c570d968c99567%40postgresql.org
Discussion: https://postgr.es/m/YFBcRbnBiPdGZvfW@paquier.xyz
Backpatch-through: 13

diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 28933f8bbe124c98a5b70e0ef6ad1fd81ae250ba..06b57ae71f18657dfe9df494a61e779c92e083dc 100644 (file)
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -801,17 +801,20 @@ durable_unlink(const char *fname, int elevel)
 }
 
 /*
- * durable_rename_excl -- rename a file in a durable manner, without
- * overwriting an existing target file
+ * durable_rename_excl -- rename a file in a durable manner.
  *
- * Similar to durable_rename(), except that this routine will fail if the
- * target file already exists.
+ * Similar to durable_rename(), except that this routine tries (but does not
+ * guarantee) not to overwrite the target file.
  *
  * Note that a crash in an unfortunate moment can leave you with two links to
  * the target file.
  *
  * Log errors with the caller specified severity.
  *
+ * On Windows, using a hard link followed by unlink() causes concurrency
+ * issues, while a simple rename() does not cause that, so be careful when
+ * changing the logic of this routine.
+ *
  * Returns 0 if the operation succeeded, -1 otherwise. Note that errno is not
  * valid upon return.
  */
@@ -825,6 +828,7 @@ durable_rename_excl(const char *oldfile, const char *newfile, int elevel)
    if (fsync_fname_ext(oldfile, false, false, elevel) != 0)
        return -1;
 
+#ifdef HAVE_WORKING_LINK
    if (link(oldfile, newfile) < 0)
    {
        ereport(elevel,
@@ -834,6 +838,16 @@ durable_rename_excl(const char *oldfile, const char *newfile, int elevel)
        return -1;
    }
    unlink(oldfile);
+#else
+   if (rename(oldfile, newfile) < 0)
+   {
+       ereport(elevel,
+               (errcode_for_file_access(),
+                errmsg("could not rename file \"%s\" to \"%s\": %m",
+                       oldfile, newfile)));
+       return -1;
+   }
+#endif
 
    /*
     * Make change persistent in case of an OS crash, both the new entry and

diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h
index f10ad0acd6fb63ac0e3eeda65522cfb55bd5abf4..e28c9903823b8965b4f039341f70310b64339435 100644 (file)
--- a/src/include/pg_config_manual.h
+++ b/src/include/pg_config_manual.h
@@ -135,6 +135,13 @@
 #define EXEC_BACKEND
 #endif
 
+/*
+ * Define this if your operating system supports link()
+ */
+#if !defined(WIN32) && !defined(__CYGWIN__)
+#define HAVE_WORKING_LINK 1
+#endif
+
 /*
  * USE_POSIX_FADVISE controls whether Postgres will attempt to use the
  * posix_fadvise() kernel call.  Usually the automatic configure tests are