Prevent race condition while reading relmapper file.

Contrary to the comment here, POSIX does not guarantee atomicity of a
read(), if another process calls write() concurrently. Or at least Linux
does not. Add locking to load_relmap_file() to avoid the race condition.

Fixes bug #17064. Thanks to Alexander Lakhin for the report and test case.

Backpatch-through: 9.6, all supported versions.
Discussion: https://www.postgresql.org/message-id/17064-bb0d7904ef72add3@postgresql.org

diff --git a/src/backend/utils/cache/relmapper.c b/src/backend/utils/cache/relmapper.c
index 424624cf0dad2a7d14eecdc7ba4ae149965ad117..34363b70c2019494f21f287bf2f6d311f29a35d5 100644 (file)
--- a/src/backend/utils/cache/relmapper.c
+++ b/src/backend/utils/cache/relmapper.c
@@ -136,7 +136,7 @@ static void apply_map_update(RelMapFile *map, Oid relationId, Oid fileNode,
                                                         bool add_okay);
 static void merge_map_updates(RelMapFile *map, const RelMapFile *updates,
                                                          bool add_okay);
-static void load_relmap_file(bool shared);
+static void load_relmap_file(bool shared, bool lock_held);
 static void write_relmap_file(bool shared, RelMapFile *newmap,
                                                          bool write_wal, bool send_sinval, bool preserve_files,
                                                          Oid dbid, Oid tsid, const char *dbpath);
@@ -405,12 +405,12 @@ RelationMapInvalidate(bool shared)
        if (shared)
        {
                if (shared_map.magic == RELMAPPER_FILEMAGIC)
-                       load_relmap_file(true);
+                       load_relmap_file(true, false);
        }
        else
        {
                if (local_map.magic == RELMAPPER_FILEMAGIC)
-                       load_relmap_file(false);
+                       load_relmap_file(false, false);
        }
 }
 
@@ -425,9 +425,9 @@ void
 RelationMapInvalidateAll(void)
 {
        if (shared_map.magic == RELMAPPER_FILEMAGIC)
-               load_relmap_file(true);
+               load_relmap_file(true, false);
        if (local_map.magic == RELMAPPER_FILEMAGIC)
-               load_relmap_file(false);
+               load_relmap_file(false, false);
 }
 
 /*
@@ -612,7 +612,7 @@ RelationMapInitializePhase2(void)
        /*
         * Load the shared map file, die on error.
         */
-       load_relmap_file(true);
+       load_relmap_file(true, false);
 }
 
 /*
@@ -633,7 +633,7 @@ RelationMapInitializePhase3(void)
        /*
         * Load the local map file, die on error.
         */
-       load_relmap_file(false);
+       load_relmap_file(false, false);
 }
 
 /*
@@ -695,7 +695,7 @@ RestoreRelationMap(char *startAddress)
  * Note that the local case requires DatabasePath to be set up.
  */
 static void
-load_relmap_file(bool shared)
+load_relmap_file(bool shared, bool lock_held)
 {
        RelMapFile *map;
        char            mapfilename[MAXPGPATH];
@@ -725,12 +725,15 @@ load_relmap_file(bool shared)
                                                mapfilename)));
 
        /*
-        * Note: we could take RelationMappingLock in shared mode here, but it
-        * seems unnecessary since our read() should be atomic against any
-        * concurrent updater's write().  If the file is updated shortly after we
-        * look, the sinval signaling mechanism will make us re-read it before we
-        * are able to access any relation that's affected by the change.
+        * 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
+        * shortly after we look, the sinval signaling mechanism will make us
+        * re-read it before we are able to access any relation that's affected by
+        * the change.
         */
+       if (!lock_held)
+               LWLockAcquire(RelationMappingLock, LW_SHARED);
+
        pgstat_report_wait_start(WAIT_EVENT_RELATION_MAP_READ);
        r = read(fd, map, sizeof(RelMapFile));
        if (r != sizeof(RelMapFile))
@@ -747,6 +750,9 @@ load_relmap_file(bool shared)
        }
        pgstat_report_wait_end();
 
+       if (!lock_held)
+               LWLockRelease(RelationMappingLock);
+
        if (CloseTransientFile(fd) != 0)
                ereport(FATAL,
                                (errcode_for_file_access(),
@@ -969,7 +975,7 @@ perform_relmap_update(bool shared, const RelMapFile *updates)
        LWLockAcquire(RelationMappingLock, LW_EXCLUSIVE);
 
        /* Be certain we see any other updates just made */
-       load_relmap_file(shared);
+       load_relmap_file(shared, true);
 
        /* Prepare updated data in a local variable */
        if (shared)