Re: Two pg_rewind patches (auto generate recovery conf and ensure clean shutdown)

[Michael Paquier]

On Wed, Oct 02, 2019 at 08:28:09PM +0300, Alexey Kondratov wrote:
> I've directly followed your guess and tried to elaborate pg_rewind test
> cases and... It seems I've caught a few bugs:
> 
> 1) --dry-run actually wasn't completely 'dry'. It did update target
> controlfile, which could cause repetitive pg_rewind calls to fail after
> dry-run ones.

I have just paid attention to this thread, but this is a bug which
goes down to 12 actually so let's treat it independently of the rest.
The control file was not written thanks to the safeguards in
write_target_range() in past versions, but the recent refactoring
around control file handling broke that promise.  Another thing which
is not completely exact is the progress reporting which should be
reported even if the dry-run mode runs.  That's less critical, but
let's make things consistent.

Patch 0001 also forgot that recovery.conf should not be written either
when no rewind is needed.

I have reworked your first patch as per the attached.  What do you
think about it?  The part with the control file needs to go down to
v12, and I would likely split that into two commits on HEAD: one for
the control file and a second for the recovery.conf portion with the
fix for --no-ensure-shutdown to keep a cleaner history.

+               # Check that incompatible options error out.
+               command_fails(
+                       [
+                               'pg_rewind', "--debug",
+                               "--source-pgdata=$standby_pgdata",
+                               "--target-pgdata=$master_pgdata", "-R",
+                               "--no-ensure-shutdown"
+                       ],
+                       'pg_rewind local with -R');
Incompatible options had better be checked within a separate perl
script?  We generally do that for the other binaries.
--
Michael

diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index a7fd9e0cab..2cc32cd404 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -38,6 +38,7 @@ static void createBackupLabel(XLogRecPtr startpoint, TimeLineID starttli,
 
 static void digestControlFile(ControlFileData *ControlFile, char *source,
 							  size_t size);
+static void updateControlFile(ControlFileData *ControlFile);
 static void syncTargetDirectory(void);
 static void sanityChecks(void);
 static void findCommonAncestorTimeline(XLogRecPtr *recptr, int *tliIndex);
@@ -101,7 +102,7 @@ main(int argc, char **argv)
 		{"write-recovery-conf", no_argument, NULL, 'R'},
 		{"source-pgdata", required_argument, NULL, 1},
 		{"source-server", required_argument, NULL, 2},
-		{"no-ensure-shutdown", no_argument, NULL, 44},
+		{"no-ensure-shutdown", no_argument, NULL, 4},
 		{"version", no_argument, NULL, 'V'},
 		{"dry-run", no_argument, NULL, 'n'},
 		{"no-sync", no_argument, NULL, 'N'},
@@ -341,7 +342,7 @@ main(int argc, char **argv)
 	if (!rewind_needed)
 	{
 		pg_log_info("no rewind required");
-		if (writerecoveryconf)
+		if (writerecoveryconf && !dry_run)
 			WriteRecoveryConfig(conn, datadir_target,
 								GenerateRecoveryConfig(conn, NULL));
 		exit(0);
@@ -435,13 +436,13 @@ main(int argc, char **argv)
 	ControlFile_new.minRecoveryPoint = endrec;
 	ControlFile_new.minRecoveryPointTLI = endtli;
 	ControlFile_new.state = DB_IN_ARCHIVE_RECOVERY;
-	update_controlfile(datadir_target, &ControlFile_new, do_sync);
+	updateControlFile(&ControlFile_new);
 
 	if (showprogress)
 		pg_log_info("syncing target data directory");
 	syncTargetDirectory();
 
-	if (writerecoveryconf)
+	if (writerecoveryconf && !dry_run)
 		WriteRecoveryConfig(conn, datadir_target,
 							GenerateRecoveryConfig(conn, NULL));
 
@@ -782,6 +783,22 @@ digestControlFile(ControlFileData *ControlFile, char *src, size_t size)
 	checkControlFile(ControlFile);
 }
 
+/*
+ * Update the target's control file.
+ */
+static void
+updateControlFile(ControlFileData *ControlFile)
+{
+	/* update progress report with control file size */
+	fetch_done += PG_CONTROL_FILE_SIZE;
+	progress_report(false);
+
+	if (dry_run)
+		return;
+
+	update_controlfile(datadir_target, ControlFile, do_sync);
+}
+
 /*
  * Sync target data directory to ensure that modifications are safely on disk.
  *

signature.asc
Description: PGP signature