Refactor pg_rewind for more clear decision making.

Deciding what to do with each file is now a separate step after all the
necessary information has been gathered. It is more clear that way.
Previously, the decision-making was divided between process_source_file()
and process_target_file(), and it was a bit hard to piece together what
the overall rules were.

Reviewed-by: Kyotaro Horiguchi, Soumyadeep Chakraborty
Discussion: https://www.postgresql.org/message-id/0c5b3783-af52-3ee5-f8fa-6e794061f70d%40iki.fi

src/bin/pg_rewind/copy_fetch.c

@@ -210,7 +210,7 @@ copy_executeFileMap(filemap_t *map)
 	for (i = 0; i < map->narray; i++)
 	{
 		entry = map->array[i];
-		execute_pagemap(&entry->pagemap, entry->path);
+		execute_pagemap(&entry->target_pages_to_overwrite, entry->path);
 
 		switch (entry->action)
 		{
@@ -219,16 +219,16 @@ copy_executeFileMap(filemap_t *map)
 				break;
 
 			case FILE_ACTION_COPY:
-				rewind_copy_file_range(entry->path, 0, entry->newsize, true);
+				rewind_copy_file_range(entry->path, 0, entry->source_size, true);
 				break;
 
 			case FILE_ACTION_TRUNCATE:
-				truncate_target_file(entry->path, entry->newsize);
+				truncate_target_file(entry->path, entry->source_size);
 				break;
 
 			case FILE_ACTION_COPY_TAIL:
-				rewind_copy_file_range(entry->path, entry->oldsize,
-									   entry->newsize, false);
+				rewind_copy_file_range(entry->path, entry->target_size,
+									   entry->source_size, false);
 				break;
 
 			case FILE_ACTION_CREATE:
@@ -238,6 +238,10 @@ copy_executeFileMap(filemap_t *map)
 			case FILE_ACTION_REMOVE:
 				remove_target(entry);
 				break;
+
+			case FILE_ACTION_UNDECIDED:
+				pg_fatal("no action decided for \"%s\"", entry->path);
+				break;
 		}
 	}
 

src/bin/pg_rewind/file_ops.c

@@ -126,8 +126,9 @@ void
 remove_target(file_entry_t *entry)
 {
 	Assert(entry->action == FILE_ACTION_REMOVE);
+	Assert(entry->target_exists);
 
-	switch (entry->type)
+	switch (entry->target_type)
 	{
 		case FILE_TYPE_DIRECTORY:
 			remove_target_dir(entry->path);
@@ -140,6 +141,10 @@ remove_target(file_entry_t *entry)
 		case FILE_TYPE_SYMLINK:
 			remove_target_symlink(entry->path);
 			break;
+
+		case FILE_TYPE_UNDEFINED:
+			pg_fatal("undefined file type for \"%s\"", entry->path);
+			break;
 	}
 }
 
@@ -147,21 +152,26 @@ void
 create_target(file_entry_t *entry)
 {
 	Assert(entry->action == FILE_ACTION_CREATE);
+	Assert(!entry->target_exists);
 
-	switch (entry->type)
+	switch (entry->source_type)
 	{
 		case FILE_TYPE_DIRECTORY:
 			create_target_dir(entry->path);
 			break;
 
 		case FILE_TYPE_SYMLINK:
-			create_target_symlink(entry->path, entry->link_target);
+			create_target_symlink(entry->path, entry->source_link_target);
 			break;
 
 		case FILE_TYPE_REGULAR:
 			/* can't happen. Regular files are created with open_target_file. */
 			pg_fatal("invalid action (CREATE) for regular file");
 			break;
+
+		case FILE_TYPE_UNDEFINED:
+			pg_fatal("undefined file type for \"%s\"", entry->path);
+			break;
 	}
 }
 

src/bin/pg_rewind/filemap.h

@@ -14,17 +14,22 @@
 
 /*
  * For every file found in the local or remote system, we have a file entry
- * which says what we are going to do with the file. For relation files,
- * there is also a page map, marking pages in the file that were changed
- * locally.
- *
- * The enum values are sorted in the order we want actions to be processed.
+ * that contains information about the file on both systems.  For relation
+ * files, there is also a page map that marks pages in the file that were
+ * changed in the target after the last common checkpoint.  Each entry also
+ * contains an 'action' field, which says what we are going to do with the
+ * file.
  */
+
+/* these enum values are sorted in the order we want actions to be processed */
 typedef enum
 {
+	FILE_ACTION_UNDECIDED = 0,	/* not decided yet */
+
 	FILE_ACTION_CREATE,			/* create local directory or symbolic link */
 	FILE_ACTION_COPY,			/* copy whole file, overwriting if exists */
-	FILE_ACTION_COPY_TAIL,		/* copy tail from 'oldsize' to 'newsize' */
+	FILE_ACTION_COPY_TAIL,		/* copy tail from 'source_size' to
+								 * 'target_size' */
 	FILE_ACTION_NONE,			/* no action (we might still copy modified
 								 * blocks based on the parsed WAL) */
 	FILE_ACTION_TRUNCATE,		/* truncate local file to 'newsize' bytes */
@@ -33,6 +38,8 @@ typedef enum
 
 typedef enum
 {
+	FILE_TYPE_UNDEFINED = 0,
+
 	FILE_TYPE_REGULAR,
 	FILE_TYPE_DIRECTORY,
 	FILE_TYPE_SYMLINK
@@ -41,19 +48,34 @@ typedef enum
 typedef struct file_entry_t
 {
 	char	   *path;
-	file_type_t type;
+	bool		isrelfile;		/* is it a relation data file? */
 
-	file_action_t action;
+	/*
+	 * Status of the file in the target.
+	 */
+	bool		target_exists;
+	file_type_t target_type;
+	size_t		target_size;	/* for a regular file */
+	char	   *target_link_target; /* for a symlink */
 
-	/* for a regular file */
-	size_t		oldsize;
-	size_t		newsize;
-	bool		isrelfile;		/* is it a relation data file? */
+	/*
+	 * Pages that were modified in the target and need to be replaced from the
+	 * source.
+	 */
+	datapagemap_t target_pages_to_overwrite;
 
-	datapagemap_t pagemap;
+	/*
+	 * Status of the file in the source.
+	 */
+	bool		source_exists;
+	file_type_t source_type;
+	size_t		source_size;
+	char	   *source_link_target; /* for a symlink */
 
-	/* for a symlink */
-	char	   *link_target;
+	/*
+	 * What will we do to the file?
+	 */
+	file_action_t action;
 
 	struct file_entry_t *next;
 } file_entry_t;
@@ -71,19 +93,18 @@ typedef struct filemap_t
 	/*
 	 * After processing all the remote files, the entries in the linked list
 	 * are moved to this array. After processing local files, too, all the
-	 * local entries are added to the array by filemap_finalize, and sorted in
-	 * the final order. After filemap_finalize, all the entries are in the
-	 * array, and the linked list is empty.
+	 * local entries are added to the array by decide_file_actions(), and
+	 * sorted in the final order. After decide_file_actions(), all the entries
+	 * are in the array, and the linked list is empty.
 	 */
 	file_entry_t **array;
 	int			narray;			/* current length of array */
 
 	/*
-	 * Summary information. total_size is the total size of the source
-	 * cluster, and fetch_size is the number of bytes that needs to be copied.
+	 * Summary information.
 	 */
-	uint64		total_size;
-	uint64		fetch_size;
+	uint64		total_size;		/* total size of the source cluster */
+	uint64		fetch_size;		/* number of bytes that needs to be copied */
 } filemap_t;
 
 extern filemap_t *filemap;
@@ -94,11 +115,12 @@ extern void print_filemap(void);
 
 /* Functions for populating the filemap */
 extern void process_source_file(const char *path, file_type_t type,
-								size_t newsize, const char *link_target);
+								size_t size, const char *link_target);
 extern void process_target_file(const char *path, file_type_t type,
-								size_t newsize, const char *link_target);
-extern void process_block_change(ForkNumber forknum, RelFileNode rnode,
-								 BlockNumber blkno);
-extern void filemap_finalize(void);
+								size_t size, const char *link_target);
+extern void process_target_wal_block_change(ForkNumber forknum,
+											RelFileNode rnode,
+											BlockNumber blkno);
+extern void decide_file_actions(void);
 
 #endif							/* FILEMAP_H */

src/bin/pg_rewind/libpq_fetch.c

@@ -465,7 +465,7 @@ libpq_executeFileMap(filemap_t *map)
 		entry = map->array[i];
 
 		/* If this is a relation file, copy the modified blocks */
-		execute_pagemap(&entry->pagemap, entry->path);
+		execute_pagemap(&entry->target_pages_to_overwrite, entry->path);
 
 		switch (entry->action)
 		{
@@ -476,15 +476,15 @@ libpq_executeFileMap(filemap_t *map)
 			case FILE_ACTION_COPY:
 				/* Truncate the old file out of the way, if any */
 				open_target_file(entry->path, true);
-				fetch_file_range(entry->path, 0, entry->newsize);
+				fetch_file_range(entry->path, 0, entry->source_size);
 				break;
 
 			case FILE_ACTION_TRUNCATE:
-				truncate_target_file(entry->path, entry->newsize);
+				truncate_target_file(entry->path, entry->source_size);
 				break;
 
 			case FILE_ACTION_COPY_TAIL:
-				fetch_file_range(entry->path, entry->oldsize, entry->newsize);
+				fetch_file_range(entry->path, entry->target_size, entry->source_size);
 				break;
 
 			case FILE_ACTION_REMOVE:
@@ -494,6 +494,10 @@ libpq_executeFileMap(filemap_t *map)
 			case FILE_ACTION_CREATE:
 				create_target(entry);
 				break;
+
+			case FILE_ACTION_UNDECIDED:
+				pg_fatal("no action decided for \"%s\"", entry->path);
+				break;
 		}
 	}
 

src/bin/pg_rewind/parsexlog.c

@@ -436,6 +436,6 @@ extractPageInfo(XLogReaderState *record)
 		if (forknum != MAIN_FORKNUM)
 			continue;
 
-		process_block_change(forknum, rnode, blkno);
+		process_target_wal_block_change(forknum, rnode, blkno);
 	}
 }

src/bin/pg_rewind/pg_rewind.c

@@ -369,7 +369,7 @@ main(int argc, char **argv)
 				chkpttli);
 
 	/*
-	 * Build the filemap, by comparing the source and target data directories.
+	 * Collect information about all files in the target and source systems.
 	 */
 	filemap_create();
 	if (showprogress)
@@ -390,8 +390,12 @@ main(int argc, char **argv)
 		pg_log_info("reading WAL in target");
 	extractPageMap(datadir_target, chkptrec, lastcommontliIndex,
 				   ControlFile_target.checkPoint, restore_command);
-	filemap_finalize();
 
+	/*
+	 * We have collected all information we need from both systems. Decide
+	 * what to do with each file.
+	 */
+	decide_file_actions();
 	if (showprogress)
 		calculate_totals();