Fix read_relmap_file() concurrency on Windows.

Commit d8cd0c6c95c0120168df93aae095df4e0682a08a introduced a file
rename that could fail on Windows, probably due to other backends
having an open file handle to the old file of the same name.
Re-arrange the locking slightly to prevent that, by making sure the
open() and close() run while we hold the lock.

Thomas Munro. I added an explanatory comment.

Discussion: https://postgr.es/m/CA%2BhUKGLZtCTgp4NTWV-wGbR2Nyag71%3DEfYTKjDKnk%2BfkhuFMHw%40mail.gmail.com

diff --git a/src/backend/utils/cache/relmapper.c b/src/backend/utils/cache/relmapper.c
index 79e09181b6bf6f35a9b2d5f0e2c6c88868d4fc52..e7345e1410fbec718d8b48400a2c4cc40ca2ee5b 100644 (file)
--- a/src/backend/utils/cache/relmapper.c
+++ b/src/backend/utils/cache/relmapper.c
@@ -788,16 +788,6 @@ read_relmap_file(RelMapFile *map, char *dbpath, bool lock_held, int elevel)
 
        Assert(elevel >= ERROR);
 
-       /* Open the target file. */
-       snprintf(mapfilename, sizeof(mapfilename), "%s/%s", dbpath,
-                        RELMAPPER_FILENAME);
-       fd = OpenTransientFile(mapfilename, O_RDONLY | PG_BINARY);
-       if (fd < 0)
-               ereport(elevel,
-                               (errcode_for_file_access(),
-                                errmsg("could not open file \"%s\": %m",
-                                               mapfilename)));
-
        /*
         * Grab the lock to prevent the file from being updated while we read it,
         * unless the caller is already holding the lock.  If the file is updated
@@ -808,6 +798,24 @@ read_relmap_file(RelMapFile *map, char *dbpath, bool lock_held, int elevel)
        if (!lock_held)
                LWLockAcquire(RelationMappingLock, LW_SHARED);
 
+       /*
+        * Open the target file.
+        *
+        * Because Windows isn't happy about the idea of renaming over a file
+        * that someone has open, we only open this file after acquiring the lock,
+        * and for the same reason, we close it before releasing the lock. That
+        * way, by the time write_relmap_file() acquires an exclusive lock, no
+        * one else will have it open.
+        */
+       snprintf(mapfilename, sizeof(mapfilename), "%s/%s", dbpath,
+                        RELMAPPER_FILENAME);
+       fd = OpenTransientFile(mapfilename, O_RDONLY | PG_BINARY);
+       if (fd < 0)
+               ereport(elevel,
+                               (errcode_for_file_access(),
+                                errmsg("could not open file \"%s\": %m",
+                                               mapfilename)));
+
        /* Now read the data. */
        pgstat_report_wait_start(WAIT_EVENT_RELATION_MAP_READ);
        r = read(fd, map, sizeof(RelMapFile));
@@ -825,15 +833,15 @@ read_relmap_file(RelMapFile *map, char *dbpath, bool lock_held, int elevel)
        }
        pgstat_report_wait_end();
 
-       if (!lock_held)
-               LWLockRelease(RelationMappingLock);
-
        if (CloseTransientFile(fd) != 0)
                ereport(elevel,
                                (errcode_for_file_access(),
                                 errmsg("could not close file \"%s\": %m",
                                                mapfilename)));
 
+       if (!lock_held)
+               LWLockRelease(RelationMappingLock);
+
        /* check for correct magic number, etc */
        if (map->magic != RELMAPPER_FILEMAGIC ||
                map->num_mappings < 0 ||