Refactor code for reading and writing relation map files.

robertmhaas · robertmhaas · commit 39f0c4bd670c · 2022-03-17T13:21:07.000-04:00
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
@@ -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);
 
