code cleanup, add comments

src/backup.c

@@ -315,7 +315,10 @@ do_backup_instance(PGconn *backup_conn, PGNodeInfo *nodeInfo)
 	dir_list_file(backup_files_list, instance_config.pgdata,
 				  true, true, false, 0, FIO_DB_HOST);
 
-	/* create database_map used for partial restore */
+	/*
+	 * Get database_map (name to oid) for use in partial restore feature.
+	 * It's possible that we fail and database_map will be NULL.
+	 */
 	database_map = get_database_map(pg_startbackup_conn);
 
 	/*
@@ -573,7 +576,7 @@ do_backup_instance(PGconn *backup_conn, PGNodeInfo *nodeInfo)
 	if (database_map)
 	{
 		write_database_map(&current, database_map, backup_files_list);
-		/* we don`t need it anymore */
+		/* cleanup */
 		parray_walk(database_map, db_map_entry_free);
 		parray_free(database_map);
 	}
@@ -1065,8 +1068,15 @@ pg_ptrack_support(PGconn *backup_conn)
 }
 
 /*
- * Create 'datname to Oid' map
- * Return NULL if failed to construct database_map // TODO doesn't look safe. See comment below.
+ * Fill 'datname to Oid' map
+ *
+ * This function can fail to get the map for legal reasons, e.g. missing
+ * permissions on pg_database during `backup`.
+ * As long as user do not use partial restore feature it`s fine.
+ *
+ * To avoid breaking a backward compatibility don't throw an ERROR,
+ * throw a warning instead of an error and return NULL.
+ * Caller is responsible for checking the result.
  */
 parray *
 get_database_map(PGconn *conn)
@@ -1075,18 +1085,16 @@ get_database_map(PGconn *conn)
 	parray *database_map = NULL;
 	int i;
 
-	/* TODO add a comment why we exclude template0 and template1 from the map */
+	/*
+	 * Do not include template0 and template1 to the map
+	 * as default databases that must always be restored.
+	 */
 	res = pgut_execute_extended(conn,
 						  "SELECT oid, datname FROM pg_catalog.pg_database "
 						  "WHERE datname NOT IN ('template1', 'template0')",
 						  0, NULL, true, true);
 
-
-	/* TODO How is that possible? Shouldn't instance have at least one database?
-	 * How can we distinguish case when instance only has template databases
-	 * and case of query failure?
-	 * Is it ok to ignore the failure?
-	 */
+	/* Don't error out, simply return NULL. See comment above. */
 	if (PQresultStatus(res) != PGRES_TUPLES_OK)
 	{
 		PQclear(res);
@@ -1116,15 +1124,6 @@ get_database_map(PGconn *conn)
 		parray_append(database_map, db_entry);
 	}
 
-	/* extra paranoia */
-	// TODO This code block has no value. Let's delete it.
-	if (database_map && (parray_num(database_map) == 0))
-	{
-		parray_free(database_map);
-		elog(WARNING, "Failed to get database map");
-		return NULL;
-	}
-
 	return database_map;
 }
 

src/data.c

@@ -1096,7 +1096,7 @@ create_empty_file(fio_location from_location, const char *to_root,
 	}
 
 	if (fio_fclose(out))
-		elog(ERROR, "cannot write \"%s\": %s", to_path, strerror(errno));
+		elog(ERROR, "cannot close \"%s\": %s", to_path, strerror(errno));
 
 	return true;
 }

src/dir.c

@@ -445,6 +445,22 @@ BlackListCompare(const void *str1, const void *str2)
 	return strcmp(*(char **) str1, *(char **) str2);
 }
 
+/* Compare two Oids */
+int
+pgCompareOid(const void *f1, const void *f2)
+{
+	Oid v1 = *(Oid *)f1;
+	Oid v2 = *(Oid *)f2;
+
+	elog(WARNING, "pgCompareOid %u %u", v1, v2);
+	if (v1 > v2)
+		return 1;
+	else if (v1 < v2)
+		return -1;
+	else
+		return 0;}
+
+
 void
 db_map_entry_free(void *entry)
 {
@@ -1679,7 +1695,7 @@ print_database_map(FILE *out, parray *database_map)
 }
 
 /*
- * Create file 'database_map' and add its meta to backup_content.control
+ * Create file 'database_map' and add its meta to backup_files_list
  * NULL check for database_map must be done by the caller.
  */
 void
@@ -1709,8 +1725,10 @@ write_database_map(pgBackup *backup, parray *database_map, parray *backup_files_
 	/* Add metadata to backup_content.control */
 	file = pgFileNew(database_map_path, DATABASE_MAP, true, 0,
 								 FIO_BACKUP_HOST);
-	file->crc = pgFileGetCRC(file->path, true, false,
-								&file->read_size, FIO_BACKUP_HOST);
+	pfree(file->path);
+	file->path = strdup(DATABASE_MAP);
+	file->crc = pgFileGetCRC(database_map_path, true, false,
+							 &file->read_size, FIO_BACKUP_HOST);
 	file->write_size = file->read_size;
 	parray_append(backup_files_list, file);
 }

src/pg_probackup.c

@@ -641,7 +641,6 @@ main(int argc, char *argv[])
 				if (datname_exclude_list && datname_include_list)
 					elog(ERROR, "You cannot specify '--db-include' and '--db-exclude' together");
 
-				/* At this point we are sure that user requested partial restore */
 				if (datname_exclude_list)
 					datname_list = datname_exclude_list;
 
@@ -777,7 +776,7 @@ opt_datname_exclude_list(ConfigOption *opt, const char *arg)
 
 	dbname = pgut_malloc(strlen(arg) + 1);
 
-	/* add sanity for database name */
+	/* TODO add sanity for database name */
 	strcpy(dbname, arg);
 
 	parray_append(datname_exclude_list, dbname);
@@ -794,7 +793,7 @@ opt_datname_include_list(ConfigOption *opt, const char *arg)
 
 	dbname = pgut_malloc(strlen(arg) + 1);
 
-	/* add sanity for database name */
+	/* TODO add sanity for database name */
 	strcpy(dbname, arg);
 
 	parray_append(datname_include_list, dbname);

src/pg_probackup.h

@@ -654,6 +654,7 @@ extern int pgFileComparePathDesc(const void *f1, const void *f2);
 extern int pgFileComparePathWithExternalDesc(const void *f1, const void *f2);
 extern int pgFileCompareLinked(const void *f1, const void *f2);
 extern int pgFileCompareSize(const void *f1, const void *f2);
+extern int pgCompareOid(const void *f1, const void *f2);
 
 /* in data.c */
 extern bool check_data_file(ConnectionArgs* arguments, pgFile* file, uint32 checksum_version);

src/restore.c

@@ -44,8 +44,6 @@ static void *restore_files(void *arg);
 
 static parray *get_dbOid_exclude_list(pgBackup *backup, parray *files,
 									  parray *datname_list, bool partial_restore_type);
-
-static int pgCompareOid(const void *f1, const void *f2);
 static void set_orphan_status(parray *backups, pgBackup *parent_backup);
 
 /*
@@ -482,7 +480,7 @@ do_restore_or_validate(time_t target_backup_id, pgRecoveryTarget *rt,
 		}
 
 		/*
-		 * At least restore backups files starting from the parent backup.
+		 * Restore backups files starting from the parent backup.
 		 */
 		for (i = parray_num(parent_chain) - 1; i >= 0; i--)
 		{
@@ -1185,27 +1183,38 @@ get_dbOid_exclude_list(pgBackup *backup, parray *files,
 	int j;
 	parray *database_map = NULL;
 	parray *dbOid_exclude_list = NULL;
-	bool found_database_map = false;
+	pgFile *database_map_file = NULL;
+	pg_crc32	crc;
+	char		path[MAXPGPATH];
+	char		database_map_path[MAXPGPATH];
 
 	/* make sure that database_map is in backup_content.control */
-	// TODO can't we use parray_bsearch here?
 	for (i = 0; i < parray_num(files); i++)
 	{
 		pgFile	   *file = (pgFile *) parray_get(files, i);
 
 		if ((file->external_dir_num == 0) &&
-			strcmp(DATABASE_MAP, file->rel_path) == 0)
+			strcmp(DATABASE_MAP, file->name) == 0)
 		{
-			found_database_map = true;
+			database_map_file = file;
 			break;
 		}
 	}
 
-	// TODO rephrase error message
-	if (!found_database_map)
-		elog(ERROR, "Backup %s has missing database_map, partial restore is impossible.",
+	if (!database_map_file)
+		elog(ERROR, "Backup %s doesn't contain a database_map, partial restore is impossible.",
 			base36enc(backup->start_time));
 
+	pgBackupGetPath(backup, path, lengthof(path), DATABASE_DIR);
+	join_path_components(database_map_path, path, DATABASE_MAP);
+
+	/* check database_map CRC */
+	crc = pgFileGetCRC(database_map_path, true, true, NULL, FIO_LOCAL_HOST);
+
+	if (crc != database_map_file->crc)
+		elog(ERROR, "Invalid CRC of backup file \"%s\" : %X. Expected %X",
+				database_map_file->path, crc, database_map_file->crc);
+
 	/* get database_map from file */
 	database_map = read_database_map(backup);
 
@@ -1215,12 +1224,12 @@ get_dbOid_exclude_list(pgBackup *backup, parray *files,
 			base36enc(backup->start_time));
 
 	/*
-	 * So we have a list of datnames and database_map for it.
+	 * So we have a list of datnames and a database_map for it.
 	 * We must construct a list of dbOids to exclude.
 	 */
 	if (partial_restore_type)
 	{
-		/* For 'include' find dbOid of every datname NOT specified by user */
+		/* For 'include' keep dbOid of every datname NOT specified by user */
 		for (i = 0; i < parray_num(datname_list); i++)
 		{
 			bool found_match = false;
@@ -1294,15 +1303,3 @@ get_dbOid_exclude_list(pgBackup *backup, parray *files,
 
 	return dbOid_exclude_list;
 }
-
-/* Compare two Oids */
-// TODO is it overflow safe?
-// TODO move it to dir.c like other compare functions
-int
-pgCompareOid(const void *f1, const void *f2)
-{
-	Oid *f1p = *(Oid **)f1;
-	Oid *f2p = *(Oid **)f2;
-
-	return (*(Oid*)f1p - *(Oid*)f2p);
-}