Broken --dry-run mode in pg_rewind
Hi all.
Seems, that pg_rewind does not account --dry-run option properly. A simple fix for that is attached.
--
May the force be with you…
https://simply.name
Attachments:
pg_rewind_dry_run_fix.patchapplication/octet-stream; name=pg_rewind_dry_run_fix.patchDownload
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index d3ae767..eee03bd 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -303,6 +303,9 @@ main(int argc, char **argv)
fetch_done = 0;
}
+ if (dry_run)
+ exit(0);
+
/*
* This is the point of no return. Once we start copying things, we have
* modified the target directory and there is no turning back!
On 05/08/2015 03:25 PM, Vladimir Borodin wrote:
Hi all.
Seems, that pg_rewind does not account --dry-run option properly. A simple fix
for that is attached.
No, the --dry-run takes effect later. It performs all the actions it
normally would, including reading files from the source, except for
actually writing anything in the target. See the dry-run checks in
file_ops.c
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, May 8, 2015 at 9:34 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 05/08/2015 03:25 PM, Vladimir Borodin wrote:
Seems, that pg_rewind does not account --dry-run option properly. A simple
fix
for that is attached.No, the --dry-run takes effect later. It performs all the actions it
normally would, including reading files from the source, except for actually
writing anything in the target. See the dry-run checks in file_ops.c
Even if the patch sent is incorrect, shouldn't there be some process
bypass in updateControlFile() and createBackupLabel() in case of a
--dry-run?
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 05/08/2015 03:39 PM, Michael Paquier wrote:
On Fri, May 8, 2015 at 9:34 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 05/08/2015 03:25 PM, Vladimir Borodin wrote:
Seems, that pg_rewind does not account --dry-run option properly. A simple
fix
for that is attached.No, the --dry-run takes effect later. It performs all the actions it
normally would, including reading files from the source, except for actually
writing anything in the target. See the dry-run checks in file_ops.cEven if the patch sent is incorrect, shouldn't there be some process
bypass in updateControlFile() and createBackupLabel() in case of a
--dry-run?
They both use open_target_file() and write_target_file(), which check
for --dry-run and do nothing if it's set.
Hmm, I wonder it we should print something else than "Done!" at the end,
if run in --dry-run mode. Or give some indication around the time it
says "Rewinding from last common checkpoint at ...", that it's running
in dry-run mode and won't actually modify anything. The progress
messages are a bit alarming if you don't realize that it's skipping all
the writes.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Heikki Linnakangas (hlinnaka@iki.fi) wrote:
On 05/08/2015 03:39 PM, Michael Paquier wrote:
On Fri, May 8, 2015 at 9:34 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 05/08/2015 03:25 PM, Vladimir Borodin wrote:
Seems, that pg_rewind does not account --dry-run option properly. A simple
fix
for that is attached.No, the --dry-run takes effect later. It performs all the actions it
normally would, including reading files from the source, except for actually
writing anything in the target. See the dry-run checks in file_ops.cEven if the patch sent is incorrect, shouldn't there be some process
bypass in updateControlFile() and createBackupLabel() in case of a
--dry-run?They both use open_target_file() and write_target_file(), which
check for --dry-run and do nothing if it's set.Hmm, I wonder it we should print something else than "Done!" at the
end, if run in --dry-run mode. Or give some indication around the
time it says "Rewinding from last common checkpoint at ...", that
it's running in dry-run mode and won't actually modify anything. The
progress messages are a bit alarming if you don't realize that it's
skipping all the writes.
Wouldn't hurt to also augment that rather doom-looking "point of no
return" comment with a note that says writes won't happen if in
dry-run. :)
For my 2c anyway.
Thanks!
Stephen
8 мая 2015 г., в 16:11, Stephen Frost <sfrost@snowman.net> написал(а):
* Heikki Linnakangas (hlinnaka@iki.fi <mailto:hlinnaka@iki.fi>) wrote:
On 05/08/2015 03:39 PM, Michael Paquier wrote:
On Fri, May 8, 2015 at 9:34 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 05/08/2015 03:25 PM, Vladimir Borodin wrote:
Seems, that pg_rewind does not account --dry-run option properly. A simple
fix
for that is attached.No, the --dry-run takes effect later. It performs all the actions it
normally would, including reading files from the source, except for actually
writing anything in the target. See the dry-run checks in file_ops.c
Urgh, my test script had an error and I made grep only in pg_rewind.c. Sorry for noise.
Even if the patch sent is incorrect, shouldn't there be some process
bypass in updateControlFile() and createBackupLabel() in case of a
--dry-run?They both use open_target_file() and write_target_file(), which
check for --dry-run and do nothing if it's set.Hmm, I wonder it we should print something else than "Done!" at the
end, if run in --dry-run mode. Or give some indication around the
time it says "Rewinding from last common checkpoint at ...", that
it's running in dry-run mode and won't actually modify anything. The
progress messages are a bit alarming if you don't realize that it's
skipping all the writes.
That would be really nice.
Wouldn't hurt to also augment that rather doom-looking "point of no
return" comment with a note that says writes won't happen if in
dry-run. :)For my 2c anyway.
Thanks!
Stephen
--
May the force be with you…
https://simply.name
8 мая 2015 г., в 16:39, Vladimir Borodin <root@simply.name> написал(а):
8 мая 2015 г., в 16:11, Stephen Frost <sfrost@snowman.net <mailto:sfrost@snowman.net>> написал(а):
* Heikki Linnakangas (hlinnaka@iki.fi <mailto:hlinnaka@iki.fi>) wrote:
On 05/08/2015 03:39 PM, Michael Paquier wrote:
On Fri, May 8, 2015 at 9:34 PM, Heikki Linnakangas <hlinnaka@iki.fi <mailto:hlinnaka@iki.fi>> wrote:
On 05/08/2015 03:25 PM, Vladimir Borodin wrote:
Seems, that pg_rewind does not account --dry-run option properly. A simple
fix
for that is attached.No, the --dry-run takes effect later. It performs all the actions it
normally would, including reading files from the source, except for actually
writing anything in the target. See the dry-run checks in file_ops.cUrgh, my test script had an error and I made grep only in pg_rewind.c. Sorry for noise.
Even if the patch sent is incorrect, shouldn't there be some process
bypass in updateControlFile() and createBackupLabel() in case of a
--dry-run?They both use open_target_file() and write_target_file(), which
check for --dry-run and do nothing if it's set.Hmm, I wonder it we should print something else than "Done!" at the
end, if run in --dry-run mode. Or give some indication around the
time it says "Rewinding from last common checkpoint at ...", that
it's running in dry-run mode and won't actually modify anything. The
progress messages are a bit alarming if you don't realize that it's
skipping all the writes.That would be really nice.
Added comments in all places mentioned in this thread.
Wouldn't hurt to also augment that rather doom-looking "point of no
return" comment with a note that says writes won't happen if in
dry-run. :)For my 2c anyway.
Thanks!
Stephen
--
May the force be with you…
https://simply.name <https://simply.name/>
--
May the force be with you…
https://simply.name
Attachments:
pg_rewind_dry_run_comments.patchapplication/octet-stream; name=pg_rewind_dry_run_comments.patchDownload
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index d3ae767..6effd29 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -256,6 +256,9 @@ main(int argc, char **argv)
exit(0);
}
+ if (dry_run)
+ printf(_("Running in dry-run mode. Nothing would be changed.\n"));
+
findLastCheckpoint(datadir_target, divergerec, lastcommontli,
&chkptrec, &chkpttli, &chkptredo);
printf(_("Rewinding from last common checkpoint at %X/%X on timeline %u\n"),
@@ -306,6 +309,9 @@ main(int argc, char **argv)
/*
* This is the point of no return. Once we start copying things, we have
* modified the target directory and there is no turning back!
+ *
+ * NOTE: If --dry-run option is given, nothing would be changed. See
+ * dry_run checks in file_ops.c.
*/
executeFileMap();
@@ -340,7 +346,8 @@ main(int argc, char **argv)
ControlFile_new.state = DB_IN_ARCHIVE_RECOVERY;
updateControlFile(&ControlFile_new);
- printf(_("Done!\n"));
+ if (!dry_run)
+ printf(_("Done!\n"));
return 0;
}
@@ -462,6 +469,9 @@ findCommonAncestorTimeline(XLogRecPtr *recptr, TimeLineID *tli)
/*
* Create a backup_label file that forces recovery to begin at the last common
* checkpoint.
+ *
+ * Note, that nothing would be changed if --dry-run option is given since
+ * open_target_file and other functions from file_ops.c checks dry_run.
*/
static void
createBackupLabel(XLogRecPtr startpoint, TimeLineID starttli, XLogRecPtr checkpointloc)
@@ -538,6 +548,9 @@ digestControlFile(ControlFileData *ControlFile, char *src, size_t size)
/*
* Update the target's control file.
+ *
+ * Note, that nothing would be changed if --dry-run option is given since
+ * open_target_file and other functions from file_ops.c checks dry_run.
*/
static void
updateControlFile(ControlFileData *ControlFile)