Обсуждение: 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
Вложения
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