Handle creation/removal of directory trees correctly.

If there is a directory with files or subdirectories in it in the source
that are not present in the destination, we will remove them all. But we
must be careful to remove the contents of the directory before the directory
itself, otherwise rmdir() failes with ENOTEMPTY.

Also add a test case for that.

I believe this fixes issue #34 reported by Thom Brown, where a subdir in
pg_replslot was present in source but not destination.

Makefile

@@ -10,7 +10,7 @@ PROGRAM = pg_rewind
 OBJS	= pg_rewind.o parsexlog.o xlogreader.o util.o datapagemap.o timeline.o \
 	fetch.o copy_fetch.o libpq_fetch.o filemap.o
 
-REGRESS = basictest
+REGRESS = basictest extrafiles
 EXTRA_REGRESS_OPTS=--use-existing --launcher=./launcher
 
 PG_CPPFLAGS = -I$(libpq_srcdir)

copy_fetch.c

@@ -283,9 +283,11 @@ void
 copy_executeFileMap(filemap_t *map)
 {
 	file_entry_t *entry;
+	int			i;
 
-	for (entry = map->first; entry != NULL; entry = entry->next)
+	for (i = 0; i < map->narray; i++)
 	{
+		entry = map->array[i];
 		execute_pagemap(&entry->pagemap, entry->path);
 
 		switch (entry->action)

expected/extrafiles.out

@@ -0,0 +1,11 @@
+Master initialized and running.
+Standby initialized and running.
+Standby promoted.
+Running pg_rewind...
+Old master restarted after rewind.
+tst_both_dir
+tst_both_dir/both_file1
+tst_both_dir/both_file2
+tst_standby_dir
+tst_standby_dir/standby_file1
+tst_standby_dir/standby_file2

filemap.c

@@ -21,6 +21,8 @@ filemap_t *filemap = NULL;
 
 static bool isRelDataFile(const char *path);
 static int path_cmp(const void *a, const void *b);
+static int final_filemap_cmp(const void *a, const void *b);
+static void filemap_list_to_array(void);
 
 
 /*****
@@ -35,8 +37,9 @@ filemap_create(void)
 {
 	filemap_t *map = pg_malloc(sizeof(filemap_t));
 	map->first = map->last = NULL;
+	map->nlist = 0;
 	map->array = NULL;
-	map->nfiles = 0;
+	map->narray = 0;
 
 	Assert(filemap == NULL);
 	filemap = map;
@@ -199,7 +202,7 @@ process_remote_file(const char *path, size_t newsize, bool isdir)
 	}
 	else
 		map->first = map->last = entry;
-	map->nfiles++;
+	map->nlist++;
 }
 
 
@@ -219,24 +222,18 @@ process_local_file(const char *path, size_t oldsize, bool isdir)
 	file_action_t action;
 	file_entry_t *entry;
 
-	if (map->nfiles == 0)
-	{
-		/* should not happen */
-		fprintf(stderr, "remote file list is empty\n");
-		exit(1);
-	}
 	if (map->array == NULL)
 	{
 		/* on first call, initialize lookup array */
-		int i;
-		map->array = pg_malloc(map->nfiles * sizeof(file_entry_t));
-
-		i = 0;
-		for (entry = map->first; entry != NULL; entry = entry->next)
-			map->array[i++] = entry;
-		Assert (i == map->nfiles);
+		if (map->nlist == 0)
+		{
+			/* should not happen */
+			fprintf(stderr, "remote file list is empty\n");
+			exit(1);
+		}
 
-		qsort(map->array, map->nfiles, sizeof(file_entry_t *), path_cmp);
+		filemap_list_to_array();
+		qsort(map->array, map->narray, sizeof(file_entry_t *), path_cmp);
 	}
 
 	/*
@@ -250,7 +247,7 @@ process_local_file(const char *path, size_t oldsize, bool isdir)
 
 	key.path = (char *) path;
 	key_ptr = &key;
-	exists = bsearch(&key_ptr, map->array, map->nfiles, sizeof(file_entry_t *),
+	exists = bsearch(&key_ptr, map->array, map->narray, sizeof(file_entry_t *),
 					 path_cmp) != NULL;
 
 	/* Remove any file or folder that doesn't exist in the remote system. */
@@ -270,8 +267,12 @@ process_local_file(const char *path, size_t oldsize, bool isdir)
 		entry->pagemap.bitmap = NULL;
 		entry->pagemap.bitmapsize = 0;
 
-		map->last->next = entry;
+		if (map->last == NULL)
+			map->first = entry;
+		else
+			map->last->next = entry;
 		map->last = entry;
+		map->nlist++;
 	}
 	else
 	{
@@ -283,7 +284,7 @@ process_local_file(const char *path, size_t oldsize, bool isdir)
 }
 
 /*
- * This callback  gets called while we read the old WAL, for every block that
+ * This callback gets called while we read the old WAL, for every block that
  * have changed in the local system. It makes note of all the changed blocks
  * in the pagemap of the file.
  */
@@ -309,7 +310,7 @@ process_block_change(ForkNumber forknum, RelFileNode rnode, BlockNumber blkno)
 	key_ptr = &key;
 
 	{
-		file_entry_t **e = bsearch(&key_ptr, map->array, map->nfiles,
+		file_entry_t **e = bsearch(&key_ptr, map->array, map->narray,
 								   sizeof(file_entry_t *), path_cmp);
 		if (e)
 			entry = *e;
@@ -351,6 +352,43 @@ process_block_change(ForkNumber forknum, RelFileNode rnode, BlockNumber blkno)
 	}
 }
 
+/*
+ * Convert the linked list of entries in filemap->first/last to the array,
+ * filemap->array.
+ */
+static void
+filemap_list_to_array(void)
+{
+	int			narray;
+	file_entry_t *entry,
+				*next;
+
+	if (filemap->array == NULL)
+		filemap->array = pg_malloc(filemap->nlist * sizeof(file_entry_t));
+	else
+		filemap->array = pg_realloc(filemap->array,
+									(filemap->nlist + filemap->narray) * sizeof(file_entry_t));
+
+	narray = filemap->narray;
+	for (entry = filemap->first; entry != NULL; entry = next)
+	{
+		filemap->array[narray++] = entry;
+		next = entry->next;
+		entry->next = NULL;
+	}
+	Assert (narray == filemap->nlist + filemap->narray);
+	filemap->narray = narray;
+	filemap->nlist = 0;
+	filemap->first = filemap->last = NULL;
+}
+
+void
+filemap_finalize(void)
+{
+	filemap_list_to_array();
+	qsort(filemap->array, filemap->narray, sizeof(file_entry_t *),
+		  final_filemap_cmp);
+}
 
 static const char *
 action_to_str(file_action_t action)
@@ -381,9 +419,11 @@ void
 print_filemap(void)
 {
 	file_entry_t *entry;
+	int			i;
 
-	for (entry = filemap->first; entry != NULL; entry = entry->next)
+	for (i = 0; i < filemap->narray; i++)
 	{
+		entry = filemap->array[i];
 		if (entry->action != FILE_ACTION_NONE ||
 			entry->pagemap.bitmapsize > 0)
 		{
@@ -444,3 +484,26 @@ path_cmp(const void *a, const void *b)
 	file_entry_t *fb = *((file_entry_t **) b);
 	return strcmp(fa->path, fb->path);
 }
+
+/*
+ * In the final stage, the filemap is sorted so that removals come last.
+ * From disk space usage point of view, it would be better to do removals
+ * first, but for now, safety first. If a whole directory is deleted, all
+ * files and subdirectories inside it need to removed first. On creation,
+ * parent directory needs to be created before files and directories inside
+ * it. To achieve that, the file_action_t enum is ordered so that we can
+ * just sort on that first.
+ */
+static int
+final_filemap_cmp(const void *a, const void *b)
+{
+	file_entry_t *fa = *((file_entry_t **) a);
+	file_entry_t *fb = *((file_entry_t **) b);
+
+	if (fa->action > fb->action)
+		return 1;
+	if (fa->action < fb->action)
+		return -1;
+
+	return strcmp(fa->path, fb->path);
+}

filemap.h

@@ -16,16 +16,18 @@
  * 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.
  */
 typedef enum
 {
+	FILE_ACTION_CREATEDIR,	/* create local dir */
+	FILE_ACTION_COPY,		/* copy whole file, overwriting if exists */
+	FILE_ACTION_COPY_TAIL,	/* copy tail from 'oldsize' to 'newsize' */
 	FILE_ACTION_NONE,		/* no action (we might still copy modified blocks
 							 * based on the parsed WAL) */
-	FILE_ACTION_COPY,		/* copy whole file, overwriting if exists */
-	FILE_ACTION_REMOVE,		/* remove local file */
 	FILE_ACTION_TRUNCATE,	/* truncate local file to 'newsize' bytes */
-	FILE_ACTION_COPY_TAIL,	/* copy tail from 'oldsize' to 'newsize' */
-	FILE_ACTION_CREATEDIR,	/* create local dir */
+	FILE_ACTION_REMOVE,		/* remove local file */
 	FILE_ACTION_REMOVEDIR	/* remove local dir */
 } file_action_t;
 
@@ -48,16 +50,23 @@ typedef struct file_entry_t file_entry_t;
 
 struct filemap_t
 {
+	/*
+	 * New entries are accumulated to a linked list, in process_remote_file
+	 * and process_local_file.
+	 */
 	file_entry_t *first;
 	file_entry_t *last;
+	int			nlist;
 
 	/*
-	 * When array is NULL, nfiles is the number of entries in the linked list.
-	 * Otherwise nfiles is the number of entries in the array, but there can
-	 * be more entries in the linked list.
+	 * After processing all the remote files, the entries in the linked list
+	 * are moved to this array. After processing local file, 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.
 	 */
 	file_entry_t **array;
-	int			nfiles;
+	int			narray;
 };
 
 typedef struct filemap_t filemap_t;
@@ -72,5 +81,6 @@ extern void print_filemap(void);
 extern void process_remote_file(const char *path, size_t newsize, bool isdir);
 extern void process_local_file(const char *path, size_t newsize, bool isdir);
 extern void process_block_change(ForkNumber forknum, RelFileNode rnode, BlockNumber blkno);
+extern void filemap_finalize(void);
 
 #endif   /* FILEMAP_H */

libpq_fetch.c

@@ -283,6 +283,7 @@ libpq_executeFileMap(filemap_t *map)
 	file_entry_t *entry;
 	const char *sql;
 	PGresult   *res;
+	int			i;
 
 	/*
 	 * First create a temporary table, and load it with the blocks that
@@ -308,8 +309,9 @@ libpq_executeFileMap(filemap_t *map)
 		exit(1);
 	}
 
-	for (entry = map->first; entry != NULL; entry = entry->next)
+	for (i = 0; i < map->narray; i++)
 	{
+		entry = map->array[i];
 		execute_pagemap(&entry->pagemap, entry->path);
 
 		switch (entry->action)

pg_rewind.c

@@ -240,6 +240,9 @@ main(int argc, char **argv)
 		   (uint32) (chkptrec >> 32), (uint32) chkptrec,
 		   chkpttli);
 
+	/*
+	 * Build the filemap, by comparing the remote and local data directories
+	 */
 	(void) filemap_create();
 	fetchRemoteFileList();
 	traverse_datadir(datadir_target, &process_local_file);
@@ -251,6 +254,8 @@ main(int argc, char **argv)
 	 */
 	extractPageMap(datadir_target, chkptrec, lastcommontli);
 
+	filemap_finalize();
+
 	/* XXX: this is probably too verbose even in verbose mode */
 	if (verbose)
 		print_filemap();

sql/extrafiles.sql

@@ -0,0 +1,48 @@
+#!/bin/bash
+
+# This file has the .sql extension, but it is actually launched as a shell
+# script. This contortion is necessary because pg_regress normally uses
+# psql to run the input scripts, and requires them to have the .sql
+# extension, but we use a custom launcher script that runs the scripts using
+# a shell instead.
+
+# Test how pg_rewind reacts to extra files and directories in the data dirs.
+
+TESTNAME=extrafiles
+
+. sql/config_test.sh
+
+# Create a subdir that will be present in both
+function before_standby
+{
+  mkdir $TEST_MASTER/tst_both_dir
+  echo "in both1" > $TEST_MASTER/tst_both_dir/both_file1
+  echo "in both2" > $TEST_MASTER/tst_both_dir/both_file2
+}
+
+# Create subdirs that will be present only in one data dir.
+function standby_following_master
+{
+  mkdir $TEST_STANDBY/tst_standby_dir
+  echo "in standby1" > $TEST_STANDBY/tst_standby_dir/standby_file1
+  echo "in standby2" > $TEST_STANDBY/tst_standby_dir/standby_file2
+  mkdir $TEST_MASTER/tst_master_dir
+  echo "in master1" > $TEST_MASTER/tst_master_dir/master_file1
+  echo "in master2" > $TEST_MASTER/tst_master_dir/master_file2
+}
+
+function after_promotion
+{
+  :
+}
+
+
+# See what files and directories are present after rewind.
+function after_rewind
+{
+    (cd $TEST_MASTER; find tst_* | sort)
+}
+
+
+# Run the test
+. sql/run_test.sh