pg_upgrade: Improve invalid option handling

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

pg_upgrade: Improve invalid option handling

Peter Eisentraut-6
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

0001-pg_upgrade-Improve-invalid-option-handling.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade: Improve invalid option handling

Masahiko Sawada
On Thu, Jun 13, 2019 at 5:19 PM Peter Eisentraut
<[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade: Improve invalid option handling

Peter Eisentraut-6
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


Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade: Improve invalid option handling

Daniel Gustafsson
In reply to this post by Peter Eisentraut-6
> On 13 Jun 2019, at 10:19, Peter Eisentraut <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade: Improve invalid option handling

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade: Improve invalid option handling

Daniel Gustafsson
> On 18 Jun 2019, at 10:15, Michael Paquier <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade: Improve invalid option handling

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade: Improve invalid option handling

Peter Eisentraut-6
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


Reply | Threaded
Open this post in threaded view
|

Re: pg_upgrade: Improve invalid option handling

Daniel Gustafsson
> On 19 Jun 2019, at 21:51, Peter Eisentraut <[hidden email]> 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