On Fri, Jun 03, 2022 at 10:32:27PM -0500, Justin Pryzby wrote: > On Sat, Jun 04, 2022 at 12:13:19PM +0900, Michael Paquier wrote: >> I am not so sure. My first reaction was actually to bypass the >> creation of the directories on EEXIST. > > But that breaks the original motive behind the patch I wrote - logfiles are > appended to, even if they're full of errors that were fixed before re-running > pg_upgrade.
Yep. >> But, isn't the problem different and actually older here? In the set of >> commands given by Tushar, he uses the --check option without --retain, but >> the logs are kept around after the execution of the command. It seems to me >> that there is an argument for also removing the logs if the caller of the >> command does not want to retain them. > > You're right that --check is a bit inconsistent, but I don't think we could > backpatch any change to fix it. It wouldn't matter much anyway, since the > usual workflow would be "pg_upgrade --check && pg_upgrade". In which case the > logs would end up being removed anyway. Exactly, the inconsistency in the log handling is annoying, and cleaning up the logs when --retain is not used makes sense to me when the --check command succeeds, but we should keep them if the --check fails. I don't see an argument in backpatching that either. > Hmm .. maybe what you mean is that the *tap test* should first run with > --check? Sorry for the confusion. I meant to add an extra command in the TAP test itself. I would suggest the attached patch then, to add a --check command in the test suite, with a change to clean up the logs when --check is used without --retain. -- Michael
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 6114303b52..a8b73b6c5e 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -210,6 +210,11 @@ report_clusters_compatible(void)
pg_log(PG_REPORT, "\n*Clusters are compatible*\n");
/* stops new cluster */
stop_postmaster(false);
+
+ /* Remove log files? */
+ if (!log_opts.retain)
+ (void) rmtree(log_opts.basedir, true);
+
exit(0);
}
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 55c7354ba2..23a4190abb 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -213,6 +213,14 @@ chdir ${PostgreSQL::Test::Utils::tmp_check};
# Upgrade the instance.
$oldnode->stop;
+command_ok(
+ [
+ 'pg_upgrade', '--no-sync', '-d', $oldnode->data_dir,
+ '-D', $newnode->data_dir, '-b', $oldbindir,
+ '-B', $newbindir, '-p', $oldnode->port,
+ '-P', $newnode->port, '--check'
+ ],
+ 'run of pg_upgrade --check for new instance');
command_ok(
[
'pg_upgrade', '--no-sync', '-d', $oldnode->data_dir,
signature.asc
Description: PGP signature
