On Thursday, February 13, 2025 11:28 AM Shubham Khanna
<khannashubham1...@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.
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.
Maybe we can write the error message in the format "%s cannot be used with %s"
to reduce the translation workload.
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.").
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.
Best Regards,
Hou zj
