Thread: pg_basebackup and error messages dependent on the order of the arguments

Hi,

shouldn't this give the same error message?

$ pg_basebackup --checkpoint=fast --format=t --compress --pgdata=/var/tmp/dummy
pg_basebackup: error: must specify output directory or backup target
pg_basebackup: hint: Try "pg_basebackup --help" for more information.

$ pg_basebackup --pgdata=/var/tmp/dummy  --checkpoint=fast --format=t --compress
pg_basebackup: option '--compress' requires an argument
pg_basebackup: hint: Try "pg_basebackup --help" for more information.

I don't see why the first case gives not the same message as the second, especially as the "output directory" is
specified.

Tested on v17 and devel, but I guess it was always like this.

Regards
Daniel

On 2024/09/30 20:10, Daniel Westermann (DWE) wrote:
> Hi,
> 
> shouldn't this give the same error message?
> 
> $ pg_basebackup --checkpoint=fast --format=t --compress --pgdata=/var/tmp/dummy
> pg_basebackup: error: must specify output directory or backup target
> pg_basebackup: hint: Try "pg_basebackup --help" for more information.
> 
> $ pg_basebackup --pgdata=/var/tmp/dummy  --checkpoint=fast --format=t --compress
> pg_basebackup: option '--compress' requires an argument
> pg_basebackup: hint: Try "pg_basebackup --help" for more information.
> 
> I don't see why the first case gives not the same message as the second, especially as the "output directory" is
specified.

I guess because "--pgdata=/var/tmp/dummy" is processed as the argument of
--compress option in the first case, but not in the second case.
You can see the same error if you specify other optoin requiring an argument,
such as --label, in the first case.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

"Daniel Westermann (DWE)" <daniel.westermann@dbi-services.com> writes:
> shouldn't this give the same error message?

> $ pg_basebackup --checkpoint=fast --format=t --compress --pgdata=/var/tmp/dummy
> pg_basebackup: error: must specify output directory or backup target
> pg_basebackup: hint: Try "pg_basebackup --help" for more information.

> $ pg_basebackup --pgdata=/var/tmp/dummy  --checkpoint=fast --format=t --compress
> pg_basebackup: option '--compress' requires an argument
> pg_basebackup: hint: Try "pg_basebackup --help" for more information.

> I don't see why the first case gives not the same message as the second, especially as the "output directory" is
specified.

It appears that the first case treats "--pgdata=/var/tmp/dummy"
as the argument of --compress, and it doesn't bother to check that
that specifies a valid compression algorithm until much later.

As this example shows, we really ought to validate the compression
argument on sight in order to get sensible error messages.  The
trouble is that for server-side compression the code wants to just
pass the string through to the server and not form its own opinion
as to whether it's a known algorithm.

Perhaps it would help if we simply rejected strings beginning
with a dash?  I haven't tested, but roughly along the lines of

            case 'Z':
+               /* we don't want to check the algorithm yet, but ... */
+               if (optarg[0] == '-')
+                   pg_fatal("invalid compress option \"%s\", optarg);
                backup_parse_compress_options(optarg, &compression_algorithm,
                                              &compression_detail, &compressloc);
                break;

            regards, tom lane

I wrote:
> As this example shows, we really ought to validate the compression
> argument on sight in order to get sensible error messages.  The
> trouble is that for server-side compression the code wants to just
> pass the string through to the server and not form its own opinion
> as to whether it's a known algorithm.

> Perhaps it would help if we simply rejected strings beginning
> with a dash?  I haven't tested, but roughly along the lines of

Taking a closer look, many of the other switches-requiring-an-argument
also just absorb "optarg" without checking its value till much later,
so I'm not sure how far we could move the needle by special-casing
--compress.

            regards, tom lane

"Daniel Westermann (DWE)" <daniel.westermann@dbi-services.com> writes:
>> Taking a closer look, many of the other switches-requiring-an-argument
>> also just absorb "optarg" without checking its value till much later,
>> so I'm not sure how far we could move the needle by special-casing
>> --compress.

> My point was not so much about --compress but rather giving a good error message.

Right, and my point was that the issue is bigger than --compress.
For example, you get exactly the same misbehavior with

$ pg_basebackup --checkpoint=fast --format=t -d --pgdata=/var/tmp/dummy
pg_basebackup: error: must specify output directory or backup target
pg_basebackup: hint: Try "pg_basebackup --help" for more information.

I'm not sure how to solve the problem once you consider this larger
scope.  I don't think we could forbid arguments beginning with "-" for
all of these switches.

            regards, tom lane

>> My point was not so much about --compress but rather giving a good error message.

>Right, and my point was that the issue is bigger than --compress.
>For example, you get exactly the same misbehavior with

>$ pg_basebackup --checkpoint=fast --format=t -d --pgdata=/var/tmp/dummy
>pg_basebackup: error: must specify output directory or backup target
>pg_basebackup: hint: Try "pg_basebackup --help" for more information.

>I'm not sure how to solve the problem once you consider this larger
>scope.  I don't think we could forbid arguments beginning with "-" for
>all of these switches.

It is not dependent on short or long switches:
$ pg_basebackup --checkpoint=fast --format=t -p --pgdata=/var/tmp/dummy
pg_basebackup: error: must specify output directory or backup target
pg_basebackup: hint: Try "pg_basebackup --help" for more information.

$ pg_basebackup --checkpoint=fast --format=t --port --pgdata=/var/tmp/dummy
pg_basebackup: error: must specify output directory or backup target
pg_basebackup: hint: Try "pg_basebackup --help" for more information.

Maybe checking if a valid "-D" or "--pgdata" was given and return a more generic error message would be an option?

Regards
Daniel

On Wed, Oct 2, 2024 at 6:00 AM Daniel Westermann (DWE)
<daniel.westermann@dbi-services.com> wrote:
> Maybe checking if a valid "-D" or "--pgdata" was given and return a more generic error message would be an option?

It doesn't really seem reasonable to me to make the tools guess
whether somebody left out the argument to an option that requires an
argument. Consider these equivalent cases:

$ pg_dump -t -Ft
pg_dump: error: no matching tables were found
$ initdb -c --text-search-config=english -D x
<lots of output>
running bootstrap script ... 2024-10-03 18:56:51.372 GMT [16909] LOG:
syntax error in file "/Users/robert.haas/pgsql/x/postgresql.conf" line
843, near token "-"
2024-10-03 18:56:51.372 GMT [16909] FATAL:  configuration file
"/Users/robert.haas/pgsql/x/postgresql.conf" contains errors
child process exited with exit code 1
initdb: removing data directory "x"
$ dropuser -h -e bob
dropuser: error: could not translate host name "-e" to address:
nodename nor servname provided, or not known

I assume there are similar cases that don't involve PostgreSQL at all.
I think that this kind of problem is basically normal and the code is
working as designed. It's true that in some cases we could maybe make
an intelligent guess that the user has messed up, but that's always
got to be based on knowing that something that is formally an option
to an argument looks more like a separate argument. And I'm just not
very enthusiastic about inserting heuristics like that all over the
place. It seems like a lot of work and hard to get right, and it could
easily end up being more annoying than useful, if say we accidentally
reject something that was truly intended to be an argument to the
switch rather than a second argument.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Oct 2, 2024 at 6:00 AM Daniel Westermann (DWE)
> <daniel.westermann@dbi-services.com> wrote:
>> Maybe checking if a valid "-D" or "--pgdata" was given and return a more generic error message would be an option?

> It doesn't really seem reasonable to me to make the tools guess
> whether somebody left out the argument to an option that requires an
> argument. Consider these equivalent cases:
> ...
> I assume there are similar cases that don't involve PostgreSQL at all.

Yeah.  This has to be a standard problem for anything that uses getopt
or getopt_long at all.  Unless there's a standard approach (which I've
not heard of) to resolving these ambiguities, I'm not sure that we
should try to outsmart everybody else.

In the case of getopt_long there's an additional problem, which is
that that function itself may contain heuristics that rearrange the
argument order based on what looks like a switch or not.  It's likely
that anything we did on top of that would behave differently depending
on which version of getopt_long it is.

            regards, tom lane