[Issue #146] Refactoring

Author: gsmolk

src/backup.c

@@ -156,7 +156,6 @@ do_backup_instance(PGconn *backup_conn, PGNodeInfo *nodeInfo, bool no_sync)
 	/* used for multitimeline incremental backup */
 	parray       *tli_list = NULL;
 
-
 	/* for fancy reporting */
 	time_t		start_time, end_time;
 	char		pretty_time[20];
@@ -820,9 +819,7 @@ do_backup(time_t start_time, bool no_validate,
 	/* Update backup status and other metainfo. */
 	current.status = BACKUP_STATUS_RUNNING;
 	current.start_time = start_time;
-	
-	current.note = set_backup_params ? set_backup_params->note : NULL;
-	
+
 	StrNCpy(current.program_version, PROGRAM_VERSION,
 			sizeof(current.program_version));
 
@@ -905,6 +902,10 @@ do_backup(time_t start_time, bool no_validate,
 		if (instance_config.master_conn_opt.pghost == NULL)
 			elog(ERROR, "Options for connection to master must be provided to perform backup from replica");
 
+	/* add note to backup if requested */
+	if (set_backup_params && set_backup_params->note)
+		add_note(&current, set_backup_params->note);
+
 	/* backup data */
 	do_backup_instance(backup_conn, &nodeInfo, no_sync);
 	pgut_atexit_pop(backup_cleanup, NULL);
@@ -937,8 +938,7 @@ do_backup(time_t start_time, bool no_validate,
 		(set_backup_params->ttl > 0 ||
 		 set_backup_params->expire_time > 0))
 	{
-		if (!pin_backup(&current, set_backup_params))
-			elog(ERROR, "Failed to pin the backup %s", base36enc(current.backup_id));
+		pin_backup(&current, set_backup_params);
 	}
 
 	if (!no_validate)

src/catalog.c

@@ -1534,18 +1534,23 @@ do_set_backup(const char *instance_name, time_t backup_id,
 
 	target_backup = (pgBackup *) parray_get(backup_list, 0);
 
-	if (!pin_backup(target_backup, set_backup_params))
-		elog(ERROR, "Failed to pin the backup %s", base36enc(backup_id));
+	/* Pin or unpin backup if requested */
+	if (set_backup_params->ttl >= 0 || set_backup_params->expire_time > 0)
+		pin_backup(target_backup, set_backup_params);
+
+	if (set_backup_params->note)
+		add_note(target_backup, set_backup_params->note);
 }
 
 /*
  * Set 'expire-time' attribute based on set_backup_params, or unpin backup
  * if ttl is equal to zero.
  */
-bool
+void
 pin_backup(pgBackup	*target_backup, pgSetBackupParams *set_backup_params)
 {
 
+	/* sanity, backup must have positive recovery-time */
 	if (target_backup->recovery_time <= 0)
 		elog(ERROR, "Failed to set 'expire-time' for backup %s: invalid 'recovery-time'",
 						base36enc(target_backup->backup_id));
@@ -1563,17 +1568,16 @@ pin_backup(pgBackup	*target_backup, pgSetBackupParams *set_backup_params)
 		{
 			elog(WARNING, "Backup %s is not pinned, nothing to unpin",
 									base36enc(target_backup->start_time));
-			return false;
+			return;
 		}
 		target_backup->expire_time = 0;
 	}
 	/* Pin comes from expire-time */
 	else if (set_backup_params->expire_time > 0)
 		target_backup->expire_time = set_backup_params->expire_time;
-	else if (!set_backup_params->note)
-		return false;
-
-	if (set_backup_params->note) target_backup->note = set_backup_params->note;
+	else
+		/* nothing to do */
+		return;
 
 	/* Update backup.control */
 	write_backup(target_backup);
@@ -1586,12 +1590,47 @@ pin_backup(pgBackup	*target_backup, pgSetBackupParams *set_backup_params)
 		elog(INFO, "Backup %s is pinned until '%s'", base36enc(target_backup->start_time),
 														expire_timestamp);
 	}
-	else if (set_backup_params->ttl == 0)
+	else
 		elog(INFO, "Backup %s is unpinned", base36enc(target_backup->start_time));
 
-	if (set_backup_params->note)
-		elog(INFO, "Saved note for backup %s", base36enc(target_backup->start_time));
-	return true;
+	return;
+}
+
+/*
+ * Add note to backup metadata or unset already existing note.
+ * It is a job of the caller to make sure that note is not NULL.
+ */
+void
+add_note(pgBackup *target_backup, char *note)
+{
+
+	char *note_string;
+
+	/* unset note */
+	if (pg_strcasecmp(note, "none") == 0)
+	{
+		target_backup->note = NULL;
+		elog(INFO, "Removing note from backup %s",
+				base36enc(target_backup->start_time));
+	}
+	else
+	{
+		/* Currently we do not allow string with newlines as note,
+		 * because it will break parsing of backup.control.
+		 * So if user provides string like this "aaa\nbbbbb",
+		 * we save only "aaa"
+		 * Example: tests.set_backup.SetBackupTest.test_add_note_newlines
+		 */
+		note_string = pgut_malloc(MAX_NOTE_SIZE);
+		sscanf(note, "%[^\n]", note_string);
+
+		target_backup->note = note_string;
+		elog(INFO, "Adding note to backup %s: '%s'",
+				base36enc(target_backup->start_time), target_backup->note);
+	}
+
+	/* Update backup.control */
+	write_backup(target_backup);
 }
 
 /*
@@ -1688,7 +1727,7 @@ pgBackupWriteControl(FILE *out, pgBackup *backup)
 		fio_fprintf(out, "external-dirs = '%s'\n", backup->external_dir_str);
 
 	if (backup->note)
-		fio_fprintf(out, "note = %s\n", backup->note);
+		fio_fprintf(out, "note = '%s'\n", backup->note);
 
 }
 

src/pg_probackup.c

@@ -75,10 +75,10 @@ char	   *replication_slot = NULL;
 bool		temp_slot = false;
 
 /* backup options */
-bool		backup_logs = false;
-bool		smooth_checkpoint;
-char       *remote_agent;
-static char	   *backup_note = NULL;
+bool         backup_logs = false;
+bool         smooth_checkpoint;
+char        *remote_agent;
+static char *backup_note = NULL;
 /* restore options */
 static char		   *target_time = NULL;
 static char		   *target_xid = NULL;
@@ -183,7 +183,7 @@ static ConfigOption cmd_options[] =
 	{ 'b', 183, "delete-expired",	&delete_expired,	SOURCE_CMD_STRICT },
 	{ 'b', 184, "merge-expired",	&merge_expired,		SOURCE_CMD_STRICT },
 	{ 'b', 185, "dry-run",			&dry_run,			SOURCE_CMD_STRICT },
-	{ 's', 238, "note",			&backup_note,		SOURCE_CMD_STRICT },
+	{ 's', 238, "note",				&backup_note,		SOURCE_CMD_STRICT },
 	/* restore options */
 	{ 's', 136, "recovery-target-time",	&target_time,	SOURCE_CMD_STRICT },
 	{ 's', 137, "recovery-target-xid",	&target_xid,	SOURCE_CMD_STRICT },
@@ -753,6 +753,9 @@ main(int argc, char *argv[])
 			set_backup_params->ttl = ttl;
 			set_backup_params->expire_time = expire_time;
 			set_backup_params->note = backup_note;
+
+			if (backup_note && strlen(backup_note) > MAX_NOTE_SIZE)
+				elog(ERROR, "Backup note cannot exceed %u bytes", MAX_NOTE_SIZE);
 		}
 	}
 

src/pg_probackup.h

@@ -90,6 +90,8 @@ extern const char  *PROGRAM_EMAIL;
 /* retry attempts */
 #define PAGE_READ_ATTEMPTS 100
 
+#define MAX_NOTE_SIZE 1024
+
 /* Check if an XLogRecPtr value is pointed to 0 offset */
 #define XRecOffIsNull(xlrp) \
 		((xlrp) % XLOG_BLCKSZ == 0)
@@ -391,7 +393,6 @@ struct pgBackup
 	parray			*files;			/* list of files belonging to this backup
 									 * must be populated explicitly */
 	char			*note;
-									
 };
 
 /* Recovery target for restore and validate subcommands */
@@ -435,14 +436,14 @@ typedef struct pgRestoreParams
 /* Options needed for set-backup command */
 typedef struct pgSetBackupParams
 {
-	int64	ttl; /* amount of time backup must be pinned
+	int64   ttl; /* amount of time backup must be pinned
 				  * -1 - do nothing
 				  * 0 - disable pinning
 				  */
-	time_t	expire_time; /* Point in time before which backup
+	time_t  expire_time; /* Point in time until backup
 						  * must be pinned.
 						  */
-	char *note;
+	char   *note;
 } pgSetBackupParams;
 
 typedef struct
@@ -781,8 +782,9 @@ extern void timelineInfoFree(void *tliInfo);
 extern parray *catalog_get_timelines(InstanceConfig *instance);
 extern void do_set_backup(const char *instance_name, time_t backup_id,
 							pgSetBackupParams *set_backup_params);
-extern bool pin_backup(pgBackup	*target_backup,
+extern void pin_backup(pgBackup	*target_backup,
 							pgSetBackupParams *set_backup_params);
+extern void add_note(pgBackup *target_backup, char *note);
 extern void pgBackupWriteControl(FILE *out, pgBackup *backup);
 extern void write_backup_filelist(pgBackup *backup, parray *files,
 								  const char *root, parray *external_list);