Refactor code for reading and writing relation map files.

Restructure things so that the functions which update the global
variables shared_map and local_map are separate from the functions
which just read and write relation map files without touching any
global variables.

In the new structure of things, write_relmap_file() writes a relmap
file but no longer performs global variable updates. A symmetric
function read_relmap_file() that just reads a file without changing
any global variables is added, and load_relmap_file(), which does
change the global variables, uses it as a subroutine.

Because write_relmap_file() no longer updates shared_map and
local_map, that logic is moved to perform_relmap_update(). However,
no similar logic is added to relmap_redo() even though it also calls
write_relmap_file(). That's because recovery must not rely on the
contents of the relation map, and therefore there is no need to
initialize it. In fact, doing so seems like a mistake, because we
might then manage to rely on the in-memory map where we shouldn't.

Patch by me, based on earlier work by Dilip Kumar. Reviewed by
Ashutosh Sharma.

Discussion: http://postgr.es/m/CA+TgmobQLgrt4AXsc0ru7aFFkzv=9fS-Q_yO69=k9WY67RCctg@mail.gmail.com

diff --git a/src/backend/utils/cache/relmapper.c b/src/backend/utils/cache/relmapper.c
index 4f6811f5714e6ab15a6c5440959683b9719bb5b3..4d0718f0018fd60cf34c42ea7e115722a372d4b2 100644 (file)
--- a/src/backend/utils/cache/relmapper.c
+++ b/src/backend/utils/cache/relmapper.c
@@ -137,8 +137,10 @@ static void apply_map_update(RelMapFile *map, Oid relationId, Oid fileNode,
 static void merge_map_updates(RelMapFile *map, const RelMapFile *updates,
                              bool add_okay);
 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,
+static void read_relmap_file(RelMapFile *map, char *dbpath, bool lock_held,
+                            int elevel);
+static void write_relmap_file(RelMapFile *newmap, bool write_wal,
+                             bool send_sinval, bool preserve_files,
                              Oid dbid, Oid tsid, const char *dbpath);
 static void perform_relmap_update(bool shared, const RelMapFile *updates);
 
@@ -568,9 +570,9 @@ RelationMapFinishBootstrap(void)
    Assert(pending_local_updates.num_mappings == 0);
 
    /* Write the files; no WAL or sinval needed */
-   write_relmap_file(true, &shared_map, false, false, false,
-                     InvalidOid, GLOBALTABLESPACE_OID, NULL);
-   write_relmap_file(false, &local_map, false, false, false,
+   write_relmap_file(&shared_map, false, false, false,
+                     InvalidOid, GLOBALTABLESPACE_OID, "global");
+   write_relmap_file(&local_map, false, false, false,
                      MyDatabaseId, MyDatabaseTableSpace, DatabasePath);
 }
 
@@ -687,39 +689,48 @@ RestoreRelationMap(char *startAddress)
 }
 
 /*
- * load_relmap_file -- load data from the shared or local map file
+ * load_relmap_file -- load the shared or local map file
  *
- * Because the map file is essential for access to core system catalogs,
- * failure to read it is a fatal error.
+ * Because these files are essential for access to core system catalogs,
+ * failure to load either of them is a fatal error.
  *
  * Note that the local case requires DatabasePath to be set up.
  */
 static void
 load_relmap_file(bool shared, bool lock_held)
 {
-   RelMapFile *map;
+   if (shared)
+       read_relmap_file(&shared_map, "global", lock_held, FATAL);
+   else
+       read_relmap_file(&local_map, DatabasePath, lock_held, FATAL);
+}
+
+/*
+ * read_relmap_file -- load data from any relation mapper file
+ *
+ * dbpath must be the relevant database path, or "global" for shared relations.
+ *
+ * RelationMappingLock will be acquired released unless lock_held = true.
+ *
+ * Errors will be reported at the indicated elevel, which should be at least
+ * ERROR.
+ */
+static void
+read_relmap_file(RelMapFile *map, char *dbpath, bool lock_held, int elevel)
+{
    char        mapfilename[MAXPGPATH];
    pg_crc32c   crc;
    int         fd;
    int         r;
 
-   if (shared)
-   {
-       snprintf(mapfilename, sizeof(mapfilename), "global/%s",
-                RELMAPPER_FILENAME);
-       map = &shared_map;
-   }
-   else
-   {
-       snprintf(mapfilename, sizeof(mapfilename), "%s/%s",
-                DatabasePath, RELMAPPER_FILENAME);
-       map = &local_map;
-   }
+   Assert(elevel >= ERROR);
 
-   /* Read data ... */
+   /* Open the target file. */
+   snprintf(mapfilename, sizeof(mapfilename), "%s/%s", dbpath,
+            RELMAPPER_FILENAME);
    fd = OpenTransientFile(mapfilename, O_RDONLY | PG_BINARY);
    if (fd < 0)
-       ereport(FATAL,
+       ereport(elevel,
                (errcode_for_file_access(),
                 errmsg("could not open file \"%s\": %m",
                        mapfilename)));
@@ -734,16 +745,17 @@ load_relmap_file(bool shared, bool lock_held)
    if (!lock_held)
        LWLockAcquire(RelationMappingLock, LW_SHARED);
 
+   /* Now read the data. */
    pgstat_report_wait_start(WAIT_EVENT_RELATION_MAP_READ);
    r = read(fd, map, sizeof(RelMapFile));
    if (r != sizeof(RelMapFile))
    {
        if (r < 0)
-           ereport(FATAL,
+           ereport(elevel,
                    (errcode_for_file_access(),
                     errmsg("could not read file \"%s\": %m", mapfilename)));
        else
-           ereport(FATAL,
+           ereport(elevel,
                    (errcode(ERRCODE_DATA_CORRUPTED),
                     errmsg("could not read file \"%s\": read %d of %zu",
                            mapfilename, r, sizeof(RelMapFile))));
@@ -754,7 +766,7 @@ load_relmap_file(bool shared, bool lock_held)
        LWLockRelease(RelationMappingLock);
 
    if (CloseTransientFile(fd) != 0)
-       ereport(FATAL,
+       ereport(elevel,
                (errcode_for_file_access(),
                 errmsg("could not close file \"%s\": %m",
                        mapfilename)));
@@ -763,7 +775,7 @@ load_relmap_file(bool shared, bool lock_held)
    if (map->magic != RELMAPPER_FILEMAGIC ||
        map->num_mappings < 0 ||
        map->num_mappings > MAX_MAPPINGS)
-       ereport(FATAL,
+       ereport(elevel,
                (errmsg("relation mapping file \"%s\" contains invalid data",
                        mapfilename)));
 
@@ -773,7 +785,7 @@ load_relmap_file(bool shared, bool lock_held)
    FIN_CRC32C(crc);
 
    if (!EQ_CRC32C(crc, map->crc))
-       ereport(FATAL,
+       ereport(elevel,
                (errmsg("relation mapping file \"%s\" contains incorrect checksum",
                        mapfilename)));
 }
@@ -795,16 +807,16 @@ load_relmap_file(bool shared, bool lock_held)
  *
  * Because this may be called during WAL replay when MyDatabaseId,
  * DatabasePath, etc aren't valid, we require the caller to pass in suitable
- * values.  The caller is also responsible for being sure no concurrent
- * map update could be happening.
+ * values. Pass dbpath as "global" for the shared map.
+ *
+ * The caller is also responsible for being sure no concurrent map update
+ * could be happening.
  */
 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)
+write_relmap_file(RelMapFile *newmap, bool write_wal, bool send_sinval,
+                 bool preserve_files, Oid dbid, Oid tsid, const char *dbpath)
 {
    int         fd;
-   RelMapFile *realmap;
    char        mapfilename[MAXPGPATH];
 
    /*
@@ -822,19 +834,8 @@ write_relmap_file(bool shared, RelMapFile *newmap,
     * Open the target file.  We prefer to do this before entering the
     * critical section, so that an open() failure need not force PANIC.
     */
-   if (shared)
-   {
-       snprintf(mapfilename, sizeof(mapfilename), "global/%s",
-                RELMAPPER_FILENAME);
-       realmap = &shared_map;
-   }
-   else
-   {
-       snprintf(mapfilename, sizeof(mapfilename), "%s/%s",
-                dbpath, RELMAPPER_FILENAME);
-       realmap = &local_map;
-   }
-
+   snprintf(mapfilename, sizeof(mapfilename), "%s/%s",
+            dbpath, RELMAPPER_FILENAME);
    fd = OpenTransientFile(mapfilename, O_WRONLY | O_CREAT | PG_BINARY);
    if (fd < 0)
        ereport(ERROR,
@@ -934,16 +935,6 @@ write_relmap_file(bool shared, RelMapFile *newmap,
        }
    }
 
-   /*
-    * Success, update permanent copy.  During bootstrap, we might be working
-    * on the permanent copy itself, in which case skip the memcpy() to avoid
-    * invoking nominally-undefined behavior.
-    */
-   if (realmap != newmap)
-       memcpy(realmap, newmap, sizeof(RelMapFile));
-   else
-       Assert(!send_sinval);   /* must be bootstrapping */
-
    /* Critical section done */
    if (write_wal)
        END_CRIT_SECTION();
@@ -990,10 +981,19 @@ perform_relmap_update(bool shared, const RelMapFile *updates)
    merge_map_updates(&newmap, updates, allowSystemTableMods);
 
    /* Write out the updated map and do other necessary tasks */
-   write_relmap_file(shared, &newmap, true, true, true,
+   write_relmap_file(&newmap, true, true, true,
                      (shared ? InvalidOid : MyDatabaseId),
                      (shared ? GLOBALTABLESPACE_OID : MyDatabaseTableSpace),
-                     DatabasePath);
+                     (shared ? "global" : DatabasePath));
+
+   /*
+    * We succesfully wrote the updated file, so it's now safe to rely on the
+    * new values in this process, too.
+    */
+   if (shared)
+       memcpy(&shared_map, &newmap, sizeof(RelMapFile));
+   else
+       memcpy(&local_map, &newmap, sizeof(RelMapFile));
 
    /* Now we can release the lock */
    LWLockRelease(RelationMappingLock);
@@ -1033,8 +1033,7 @@ relmap_redo(XLogReaderState *record)
         * but grab the lock to interlock against load_relmap_file().
         */
        LWLockAcquire(RelationMappingLock, LW_EXCLUSIVE);
-       write_relmap_file((xlrec->dbid == InvalidOid), &newmap,
-                         false, true, false,
+       write_relmap_file(&newmap, false, true, false,
                          xlrec->dbid, xlrec->tsid, dbpath);
        LWLockRelease(RelationMappingLock);