Review issue_79. Added several TODO comments

Author: alubennikova

src/backup.c

@@ -493,15 +493,6 @@ do_backup_instance(PGconn *backup_conn, PGNodeInfo *nodeInfo)
 		}
 	}
 
-	/* write database map to file and add it to control file */
-	if (database_map)
-	{
-		write_database_map(&current, database_map, backup_files_list);
-		/* we don`t need it anymore */
-		parray_walk(database_map, db_map_entry_free);
-		parray_free(database_map);
-	}
-
 	/* clean previous backup file list */
 	if (prev_backup_filelist)
 	{
@@ -578,6 +569,15 @@ do_backup_instance(PGconn *backup_conn, PGNodeInfo *nodeInfo)
 		parray_free(xlog_files_list);
 	}
 
+	/* write database map to file and add it to control file */
+	if (database_map)
+	{
+		write_database_map(&current, database_map, backup_files_list);
+		/* we don`t need it anymore */
+		parray_walk(database_map, db_map_entry_free);
+		parray_free(database_map);
+	}
+
 	/* Print the list of files to backup catalog */
 	write_backup_filelist(&current, backup_files_list, instance_config.pgdata,
 						  external_dirs);
@@ -1066,7 +1066,7 @@ pg_ptrack_support(PGconn *backup_conn)
 
 /*
  * Create 'datname to Oid' map
- * Return NULL if failed to construct database_map
+ * Return NULL if failed to construct database_map // TODO doesn't look safe. See comment below.
  */
 parray *
 get_database_map(PGconn *conn)
@@ -1075,11 +1075,18 @@ get_database_map(PGconn *conn)
 	parray *database_map = NULL;
 	int i;
 
+	/* TODO add a comment why we exclude template0 and template1 from the map */
 	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?
+	 */
 	if (PQresultStatus(res) != PGRES_TUPLES_OK)
 	{
 		PQclear(res);
@@ -1110,6 +1117,7 @@ get_database_map(PGconn *conn)
 	}
 
 	/* 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);

src/data.c

@@ -1078,7 +1078,7 @@ create_empty_file(fio_location from_location, const char *to_root,
 	char		to_path[MAXPGPATH];
 	FILE	   *out;
 
-	/* open backup file for write  */
+	/* open file for write  */
 	join_path_components(to_path, to_root, file->rel_path);
 	out = fio_fopen(to_path, PG_BINARY_W, to_location);
 	if (out == NULL)

src/dir.c

@@ -249,6 +249,10 @@ pgFileDelete(pgFile *file)
 	}
 }
 
+/*
+ * Read the file to compute its CRC.
+ * As a handy side effect, we return filesize via bytes_read parameter.
+ */
 pg_crc32
 pgFileGetCRC(const char *file_path, bool use_crc32c, bool raise_on_deleted,
 			 size_t *bytes_read, fio_location location)
@@ -442,12 +446,12 @@ BlackListCompare(const void *str1, const void *str2)
 }
 
 void
-db_map_entry_free(void *map)
+db_map_entry_free(void *entry)
 {
-	db_map_entry *m = (db_map_entry *) map;
+	db_map_entry *m = (db_map_entry *) entry;
 
 	free(m->datname);
-	free(map);
+	free(entry);
 }
 
 /*
@@ -1702,14 +1706,12 @@ write_database_map(pgBackup *backup, parray *database_map, parray *backup_files_
 			 database_map_path, strerror(errno));
 	}
 
-	/*  */
+	/* 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);
 	file->write_size = file->read_size;
-	free(file->path);
-	file->path = strdup(DATABASE_MAP);
 	parray_append(backup_files_list, file);
 }
 

src/restore.c

@@ -457,8 +457,11 @@ do_restore_or_validate(time_t target_backup_id, pgRecoveryTarget *rt,
 		 * It is important that we do this after(!) validation so
 		 * database_map can be trusted.
 		 * NOTE: database_map could be missing for legal reasons, e.g. missing
-		 * permissions on pg_database during `backup` and, as long as user do not request
-		 * partial restore, it`s OK
+		 * permissions on pg_database during `backup` and, as long as user
+		 * do not request partial restore, it`s OK.
+		 *
+		 * If partial restore is requested and database map doesn't exist,
+		 * throw an error.
 		 */
 		if (datname_list)
 			dbOid_exclude_list = get_dbOid_exclude_list(dest_backup, dest_files, datname_list,
@@ -709,19 +712,22 @@ restore_files(void *arg)
 				 i + 1, (unsigned long) parray_num(arguments->files),
 				 file->rel_path);
 
-		/* only files from pgdata can be skipped by partial restore */
-		if (arguments->dbOid_exclude_list &&
-			file->external_dir_num == 0)
+		/* Only files from pgdata can be skipped by partial restore */
+		if (arguments->dbOid_exclude_list && file->external_dir_num == 0)
 		{
-			/* exclude map is not empty */
+			/* Check if the file belongs to the database we exclude */
 			if (parray_bsearch(arguments->dbOid_exclude_list,
 							   &file->dbOid, pgCompareOid))
 			{
-				/* got a match, destination file will truncated */
+				/*
+				 * We cannot simply skip the file, because it may lead to
+				 * failure during WAL redo; hence, create empty file. 
+				 */
 				create_empty_file(FIO_BACKUP_HOST,
 					  instance_config.pgdata, FIO_DB_HOST, file);
 
-				elog(VERBOSE, "Exclude file due to partial restore: \"%s\"", file->rel_path);
+				elog(VERBOSE, "Exclude file due to partial restore: \"%s\"",
+					 file->rel_path);
 				continue;
 			}
 		}
@@ -1176,7 +1182,8 @@ parseRecoveryTargetOptions(const char *target_time,
 	return rt;
 }
 
-/* Return array of dbOids of databases that should not be restored
+/*
+ * Return array of dbOids of databases that should not be restored
  * Regardless of what option user used, db-include or db-exclude,
  * we always convert it into exclude_list.
  */
@@ -1191,6 +1198,7 @@ get_dbOid_exclude_list(pgBackup *backup, parray *files,
 	bool found_database_map = false;
 
 	/* 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);
@@ -1203,6 +1211,7 @@ get_dbOid_exclude_list(pgBackup *backup, parray *files,
 		}
 	}
 
+	// TODO rephrase error message
 	if (!found_database_map)
 		elog(ERROR, "Backup %s has missing database_map, partial restore is impossible.",
 			base36enc(backup->start_time));
@@ -1215,7 +1224,8 @@ get_dbOid_exclude_list(pgBackup *backup, parray *files,
 		elog(ERROR, "Backup %s has empty or mangled database_map, partial restore is impossible.",
 			base36enc(backup->start_time));
 
-	/* So we have list of datnames and database_map for it.
+	/*
+	 * So we have a list of datnames and database_map for it.
 	 * We must construct a list of dbOids to exclude.
 	 */
 	if (partial_restore_type)
@@ -1284,7 +1294,7 @@ get_dbOid_exclude_list(pgBackup *backup, parray *files,
 		}
 	}
 
-	/* extra sanity, we must be totally sure that list is not empty */
+	/* extra sanity: ensure that list is not empty */
 	if (!dbOid_exclude_list || parray_num(dbOid_exclude_list) < 1)
 		elog(ERROR, "Failed to find a match in database_map of backup %s for partial restore",
 					base36enc(backup->start_time));
@@ -1296,6 +1306,8 @@ get_dbOid_exclude_list(pgBackup *backup, parray *files,
 }
 
 /* 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)
 {