pg_upgrade: Improve invalid option handling
Currently, calling pg_upgrade with an invalid command-line option aborts
pg_upgrade but leaves a pg_upgrade_internal.log file lying around. This
patch reorder things a bit so that that file is not created until all
the options have been parsed.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-pg_upgrade-Improve-invalid-option-handling.patchtext/plain; charset=UTF-8; name=0001-pg_upgrade-Improve-invalid-option-handling.patch; x-mac-creator=0; x-mac-type=0Download
From 69a1f9259c025fc38280b41ced25571feeb90ef8 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 13 Jun 2019 10:12:34 +0200
Subject: [PATCH] pg_upgrade: Improve invalid option handling
Currently, calling pg_upgrade with an invalid command-line option
aborts pg_upgrade but leaves a pg_upgrade_internal.log file lying
around. Reorder things a bit so that that file is not created until
all the options have been parsed.
---
src/bin/pg_upgrade/option.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 171aaa8f13..873d1d07e4 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -101,9 +101,6 @@ parseCommandLine(int argc, char *argv[])
if (os_user_effective_id == 0)
pg_fatal("%s: cannot be run as root\n", os_info.progname);
- if ((log_opts.internal = fopen_priv(INTERNAL_LOG_FILE, "a")) == NULL)
- pg_fatal("could not write to log file \"%s\"\n", INTERNAL_LOG_FILE);
-
while ((option = getopt_long(argc, argv, "d:D:b:B:cj:ko:O:p:P:rs:U:v",
long_options, &optindex)) != -1)
{
@@ -205,7 +202,6 @@ parseCommandLine(int argc, char *argv[])
break;
case 'v':
- pg_log(PG_REPORT, "Running in verbose mode\n");
log_opts.verbose = true;
break;
@@ -214,12 +210,18 @@ parseCommandLine(int argc, char *argv[])
break;
default:
- pg_fatal("Try \"%s --help\" for more information.\n",
- os_info.progname);
- break;
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
+ os_info.progname);
+ exit(1);
}
}
+ if ((log_opts.internal = fopen_priv(INTERNAL_LOG_FILE, "a")) == NULL)
+ pg_fatal("could not write to log file \"%s\"\n", INTERNAL_LOG_FILE);
+
+ if (log_opts.verbose)
+ pg_log(PG_REPORT, "Running in verbose mode\n");
+
/* label start of upgrade in logfiles */
for (filename = output_files; *filename != NULL; filename++)
{
--
2.22.0
On Thu, Jun 13, 2019 at 5:19 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
Currently, calling pg_upgrade with an invalid command-line option aborts
pg_upgrade but leaves a pg_upgrade_internal.log file lying around. This
patch reorder things a bit so that that file is not created until all
the options have been parsed.
- pg_fatal("Try \"%s --help\" for more
information.\n",
- os_info.progname);
- break;
+ fprintf(stderr, _("Try \"%s --help\"
for more information.\n"),
+ os_info.progname);
+ exit(1);Why do we need to change pg_fatal() to fprintf() & exit()? It seems to
me that we can still use pg_fatal() here since we write the message to
stderr.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
On 2019-06-13 14:30, Masahiko Sawada wrote:
Why do we need to change pg_fatal() to fprintf() & exit()? It seems to
me that we can still use pg_fatal() here since we write the message to
stderr.
It just makes the output more consistent with other tools, e.g.,
old:
pg_upgrade: unrecognized option `--foo'
Try "pg_upgrade --help" for more information.
Failure, exiting
new:
pg_upgrade: unrecognized option `--foo'
Try "pg_upgrade --help" for more information.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 13 Jun 2019, at 10:19, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
Currently, calling pg_upgrade with an invalid command-line option aborts
pg_upgrade but leaves a pg_upgrade_internal.log file lying around. This
patch reorder things a bit so that that file is not created until all
the options have been parsed.
+1 on doing this.
+ if ((log_opts.internal = fopen_priv(INTERNAL_LOG_FILE, "a")) == NULL)
+ pg_fatal("could not write to log file \"%s\"\n", INTERNAL_LOG_FILE);While we’re at it, should we change this to “could not open log file” to make
the messaging more consistent across the utilities (pg_basebackup and psql both
use “could not open”)?
cheers ./daniel
On Fri, Jun 14, 2019 at 12:34:36PM +0200, Daniel Gustafsson wrote:
+ if ((log_opts.internal = fopen_priv(INTERNAL_LOG_FILE, "a")) == NULL) + pg_fatal("could not write to log file \"%s\"\n", INTERNAL_LOG_FILE);While we’re at it, should we change this to “could not open log file” to make
the messaging more consistent across the utilities (pg_basebackup and psql both
use “could not open”)?
I would suggest "could not open file \"%s\": %s" instead with a proper
strerror().
--
Michael
On 18 Jun 2019, at 10:15, Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Jun 14, 2019 at 12:34:36PM +0200, Daniel Gustafsson wrote:
+ if ((log_opts.internal = fopen_priv(INTERNAL_LOG_FILE, "a")) == NULL) + pg_fatal("could not write to log file \"%s\"\n", INTERNAL_LOG_FILE);While we’re at it, should we change this to “could not open log file” to make
the messaging more consistent across the utilities (pg_basebackup and psql both
use “could not open”)?I would suggest "could not open file \"%s\": %s" instead with a proper
strerror().
Correct, that matches how pg_basebackup and psql does it.
cheers ./daniel
On Tue, Jun 18, 2019 at 10:25:44AM +0200, Daniel Gustafsson wrote:
Correct, that matches how pg_basebackup and psql does it.
Perhaps you have a patch at hand? I can see four strings in
pg_upgrade, two in exec.c and two in option.c, which could be
improved.
--
Michael
On 2019-06-19 04:24, Michael Paquier wrote:
On Tue, Jun 18, 2019 at 10:25:44AM +0200, Daniel Gustafsson wrote:
Correct, that matches how pg_basebackup and psql does it.
Perhaps you have a patch at hand? I can see four strings in
pg_upgrade, two in exec.c and two in option.c, which could be
improved.
Committed my patch and the fixes for those error messages.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 19 Jun 2019, at 21:51, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
On 2019-06-19 04:24, Michael Paquier wrote:
On Tue, Jun 18, 2019 at 10:25:44AM +0200, Daniel Gustafsson wrote:
Correct, that matches how pg_basebackup and psql does it.
Perhaps you have a patch at hand? I can see four strings in
pg_upgrade, two in exec.c and two in option.c, which could be
improved.Committed my patch and the fixes for those error messages.
Looks good, thanks!
cheers ./daniel