Thread
Commits
-
pg_upgrade: Improve error messages
- cd917ffb9a43 12.0 landed
-
pg_upgrade: Improve invalid option handling
- aba78ab4a950 12.0 landed
-
pg_upgrade: Improve invalid option handling
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-06-13T08:19:05Z
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
-
Re: pg_upgrade: Improve invalid option handling
Masahiko Sawada <sawada.mshk@gmail.com> — 2019-06-13T12:30:34Z
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 -
Re: pg_upgrade: Improve invalid option handling
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-06-14T09:03:24Z
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
-
Re: pg_upgrade: Improve invalid option handling
Daniel Gustafsson <daniel@yesql.se> — 2019-06-14T10:34:36Z
> 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 -
Re: pg_upgrade: Improve invalid option handling
Michael Paquier <michael@paquier.xyz> — 2019-06-18T08:15:24Z
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 -
Re: pg_upgrade: Improve invalid option handling
Daniel Gustafsson <daniel@yesql.se> — 2019-06-18T08:25:44Z
> 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 -
Re: pg_upgrade: Improve invalid option handling
Michael Paquier <michael@paquier.xyz> — 2019-06-19T02:24:11Z
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
-
Re: pg_upgrade: Improve invalid option handling
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-06-19T19:51:43Z
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
-
Re: pg_upgrade: Improve invalid option handling
Daniel Gustafsson <daniel@yesql.se> — 2019-06-19T20:42:54Z
> 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