On Tue, Feb 18, 2025 at 2:43 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Thursday, February 13, 2025 11:28 AM Shubham Khanna <khannashubham1197@gmail.com> wrote:
>
> Hi,
>
> > The attached patch contains the required changes.
>
> Thanks for updating the patch. I think it's a useful feature.
>
> Here are few review comments:
>
> 1.
>
> + if (opt.all_databases)
> + {
> + pg_log_error("--all-databases specified multiple times");
> + exit(1);
> + }
>
> The check for redundant --all-databases usage seems unnecessary as multiple
> specifications do not cause harm or confusion for users. Similar server
> commands with an --all option (such as clusterdb/vacuumdb/reindexdb) do not
> report errors for it.
>
I agree with your point. Since multiple specifications of '--all' do
not cause any issues or confusion, and other similar commands (like
clusterdb, vacuumdb, and reindexdb) do not enforce this restriction, I
have removed the test case accordingly.
> 2.
> + while ((c = getopt_long(argc, argv, "ad:D:np:P:s:t:U:v",
> long_options, &option_index)) != -1)
> ...
> + if (num_dbs > 0)
> + {
> + pg_log_error("--all-databases cannot be used with --database");
> + exit(1);
> + }
>
>
> I think the check for similar wrong combinations should not be done inside the
> getopt_long() block. It should be performed after parsing to ensure all
> cases are handled which would also be consistent with the methodology followed in
> all other commands AFAICS.
>
I agree with your suggestion. To ensure consistency with other
commands, I have moved the check for conflicting options outside the
getopt_long() block. This allows us to handle all cases after parsing
is complete.
> Maybe we can write the error message in the format "%s cannot be used with %s"
> to reduce the translation workload.
>
Fixed.
> To be consistent with other error message for wrong option specification in this
> command, consider adding pg_log_error_hint("Try \"%s --help\" for more information.").
>
Fixed.
> 3.
>
> Ashutosh noted that commands like clusterdb and vacuumdb use the "--all" option
> to indicate all databases, and I also found that the same is true for
> pg_amcheck command, so I also think it's OK to use "-all" here.
>
Fixed.
The attached patch at [1] contains the suggested changes.
[1] - https://www.postgresql.org/message-id/CAHv8RjKAdrrt3-pF6yHb5gBricB9%3DD7O47Dxe39zRxKkShdpmw%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
