Обсуждение: BUG #17148: About --no-strict-names option and --quiet option of pg_amcheck command

The following bug has been logged on the website:

Bug reference:      17148
Logged by:          Chen Jiaoqian
Email address:      chenjq.jy@fujitsu.com
PostgreSQL version: 14beta3
Operating system:   Red Hat Enterprise Linux Server release 7.8
Description:

Hi, Author

    In PG14 beta3, when I use pg_amcheck command, both --no-strict-names
option and --quiet option are specified, the warning message of --database
option is not suppressed.
    The official website is described as follows:
    > --no-strict-names
    >    By default, if an argument to --database, --table, --index, or
--relation matches no objects, it is a fatal error. 
    >    This option downgrades that error to a warning. If this option is
used with --quiet, the warning will be suppressed as well.
    
    When I specify a non-existent database name for the --database option,
and specify the --no-strict-names option and the --quiet option, 
    pg_amcheck command should not return any message, but it still returns
the warning message.
    
    My steps are as follows:
        1)Install the amcheck plguin in the PG source package
../contrib/amcheck directory;
        2)In the postgres database, execute SQL "create extension
amcheck;"
        3)On the OS command line, execute pg_amcheck command, specify
--no-strict-names option and --quiet option, and specify a non-existent
database name for the --database option

    The result are as follows:
        [postgres14@localhost ~]$ pg_amcheck -p 51403 -d postgres -d db01
--no-strict-names --quiet
        pg_amcheck: warning: no connectable databases to check matching
"db01"
        [postgres14@localhost ~]$

Regards.

> On 17 Aug 2021, at 06:34, PG Bug reporting form <noreply@postgresql.org> wrote:

>    In PG14 beta3, when I use pg_amcheck command, both --no-strict-names
> option and --quiet option are specified, the warning message of --database
> option is not suppressed.
>    The official website is described as follows:
>> --no-strict-names
>>   By default, if an argument to --database, --table, --index, or
> --relation matches no objects, it is a fatal error.
>>   This option downgrades that error to a warning. If this option is
> used with --quiet, the warning will be suppressed as well.
>
>    When I specify a non-existent database name for the --database option,
> and specify the --no-strict-names option and the --quiet option,
>    pg_amcheck command should not return any message, but it still returns
> the warning message.

Agreed, in order to match the documentation the attached is required, which
also matches other invocations of log_no_match().  This should be applied
backpatched to 14 unless there are objections.

--
Daniel Gustafsson        https://vmware.com/

Julien Rouhaud also pointed out off-list that the help output and documentation
aren't in agreement about how --quiet works. In the documentation we say:

    "Print fewer messages, and less detail regarding any server errors."

But the help output takes a stronger position on this:

    $ ./bin/pg_amcheck --help |grep quiet
      -q, --quiet                     don't write any messages

The latter isn't entirely true IMO, so I think we should reconcile them with
something like this while in here and fixing things anyways:

-       printf(_("  -q, --quiet                     don't write any messages\n"));
+       printf(_("  -q, --quiet                     print fewer and less detailed messages\n"));

Thoughts?

--
Daniel Gustafsson        https://vmware.com/

Julien Rouhaud also pointed out off-list that the help output and documentation

aren't in agreement about how --quiet works. In the documentation we say:

"Print fewer messages, and less detail regarding any server errors."

But the help output takes a stronger position on this:

$ ./bin/pg_amcheck --help |grep quiet

  -q, --quiet                     don't write any messages

The latter isn't entirely true IMO, so I think we should reconcile them with

something like this while in here and fixing things anyways:

> On 17 Aug 2021, at 16:53, Euler Taveira <euler@eulerto.com> wrote:
> On Tue, Aug 17, 2021, at 8:06 AM, Daniel Gustafsson wrote:

>> ..I think we should reconcile them with
>> something like this while in here and fixing things anyways:

> I suggest that it should be a message that we already use in another binaries
> such as "do not print any output, except for errors".

Well, problem is that it’s plain not true.  If you pass --quiet --verbose you
will get a lot of output, albeit less than if not using --quiet.  Consistency
with other tools is obviously good, but only when it’s correct IMO.

--
Daniel Gustafsson        https://vmware.com/

> On 17 Aug 2021, at 16:53, Euler Taveira <euler@eulerto.com> wrote:

> On Tue, Aug 17, 2021, at 8:06 AM, Daniel Gustafsson wrote:

>> ..I think we should reconcile them with

>> something like this while in here and fixing things anyways:

> I suggest that it should be a message that we already use in another binaries

> such as "do not print any output, except for errors".

Well, problem is that it’s plain not true.  If you pass --quiet --verbose you

will get a lot of output, albeit less than if not using --quiet.  Consistency

with other tools is obviously good, but only when it’s correct IMO.

On 17.08.21 19:00, Euler Taveira wrote:
>> Well, problem is that it’s plain not true.  If you pass --quiet 
>> --verbose you
>> will get a lot of output, albeit less than if not using --quiet.  
>> Consistency
>> with other tools is obviously good, but only when it’s correct IMO.
> Indeed, it is not a good design. It should be one option --verbose that
> increases the verbosity according to a number or an enum value. --verbose=0
> means "quiet". However, that ship has sailed.

I was confused by this the other day as well.  Having all of

   -q, --quiet                     don't write any messages
   -P, --progress                  show progress information
   -v, --verbose                   write a lot of output

is surely a lot.

If you look at what --quiet does, it

1) disables logging warnings if there are no matches for object patterns 
and --no-strict-names is given, and

2) sets PQsetErrorVerbosity(free_slot->connection, PQERRORS_TERSE).

I think this both of these things could be deleted and we could get rid 
of the --quiet option, to simplify all this.  Neither of these behaviors 
is in common with any other PostgreSQL tool.

> On 18 Aug 2021, at 13:44, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:

> I think this both of these things could be deleted and we could get rid of the --quiet option, to simplify all this.

It simplifies the pg_amcheck code a bit, but it at the same time complicates
the tests as they are currently written.  Not sure that we want to change that
much as this point in the 14 cycle?

> Neither of these behaviors is in common with any other PostgreSQL tool.

That I agree with.

--
Daniel Gustafsson        https://vmware.com/

Daniel Gustafsson <daniel@yesql.se> writes:
>> On 18 Aug 2021, at 13:44, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>> I think this both of these things could be deleted and we could get rid of the --quiet option, to simplify all this.


> It simplifies the pg_amcheck code a bit, but it at the same time complicates
> the tests as they are currently written.  Not sure that we want to change that
> much as this point in the 14 cycle?

It's going to become much harder to change pg_amcheck's user-visible
behavior once it's shipped in a release.  Better to fix it now while
there are not backwards-compatibility concerns.

            regards, tom lane

On Wed, Aug 18, 2021 at 3:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
> >> On 18 Aug 2021, at 13:44, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
> >> I think this both of these things could be deleted and we could get rid of the --quiet option, to simplify all
this.
>
> > It simplifies the pg_amcheck code a bit, but it at the same time complicates
> > the tests as they are currently written.  Not sure that we want to change that
> > much as this point in the 14 cycle?
>
> It's going to become much harder to change pg_amcheck's user-visible
> behavior once it's shipped in a release.  Better to fix it now while
> there are not backwards-compatibility concerns.

+<several>. Let's just get it done now. I doubt many people have had
the time to integrate it into their scripts and such yet, and since
it's a beta...

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

> On 18 Aug 2021, at 15:46, Magnus Hagander <magnus@hagander.net> wrote:
>
> On Wed, Aug 18, 2021 at 3:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Daniel Gustafsson <daniel@yesql.se> writes:
>>>> On 18 Aug 2021, at 13:44, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>>>> I think this both of these things could be deleted and we could get rid of the --quiet option, to simplify all
this.
>>
>>> It simplifies the pg_amcheck code a bit, but it at the same time complicates
>>> the tests as they are currently written.  Not sure that we want to change that
>>> much as this point in the 14 cycle?
>>
>> It's going to become much harder to change pg_amcheck's user-visible
>> behavior once it's shipped in a release.  Better to fix it now while
>> there are not backwards-compatibility concerns.
>
> +<several>. Let's just get it done now. I doubt many people have had
> the time to integrate it into their scripts and such yet, and since
> it's a beta...

Since there is consensus for removing --quiet, I’ll propose a patch in a bit
for removing it and fixing up the tests.

--
Daniel Gustafsson        https://vmware.com/

> On 18 Aug 2021, at 15:49, Daniel Gustafsson <daniel@yesql.se> wrote:
>
>> On 18 Aug 2021, at 15:46, Magnus Hagander <magnus@hagander.net> wrote:
>>
>> On Wed, Aug 18, 2021 at 3:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>
>>> Daniel Gustafsson <daniel@yesql.se> writes:
>>>>> On 18 Aug 2021, at 13:44, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>>>>> I think this both of these things could be deleted and we could get rid of the --quiet option, to simplify all
this.
>>>
>>>> It simplifies the pg_amcheck code a bit, but it at the same time complicates
>>>> the tests as they are currently written.  Not sure that we want to change that
>>>> much as this point in the 14 cycle?
>>>
>>> It's going to become much harder to change pg_amcheck's user-visible
>>> behavior once it's shipped in a release.  Better to fix it now while
>>> there are not backwards-compatibility concerns.
>>
>> +<several>. Let's just get it done now. I doubt many people have had
>> the time to integrate it into their scripts and such yet, and since
>> it's a beta...
>
> Since there is consensus for removing --quiet, I’ll propose a patch in a bit
> for removing it and fixing up the tests.

Turns I was a bit undercaffeinated when skimming the tests, it really wasn’t
that invasive.  The attached diff removes --quiet and fixes up the tests and
docs to match.  Anyone who wants to keep --quiet should..  ehm, not keep quiet.

--
Daniel Gustafsson        https://vmware.com/

On Thu, Aug 19, 2021 at 2:53 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 18 Aug 2021, at 15:49, Daniel Gustafsson <daniel@yesql.se> wrote:
> >
> > Since there is consensus for removing --quiet, I’ll propose a patch in a bit
> > for removing it and fixing up the tests.
>
> Turns I was a bit undercaffeinated when skimming the tests, it really wasn’t
> that invasive.  The attached diff removes --quiet and fixes up the tests and
> docs to match.  Anyone who wants to keep --quiet should..  ehm, not keep quiet.

The patch looks good to me.

> On 19 Aug 2021, at 04:45, Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Thu, Aug 19, 2021 at 2:53 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>>
>>> On 18 Aug 2021, at 15:49, Daniel Gustafsson <daniel@yesql.se> wrote:
>>>
>>> Since there is consensus for removing --quiet, I’ll propose a patch in a bit
>>> for removing it and fixing up the tests.
>>
>> Turns I was a bit undercaffeinated when skimming the tests, it really wasn’t
>> that invasive.  The attached diff removes --quiet and fixes up the tests and
>> docs to match.  Anyone who wants to keep --quiet should..  ehm, not keep quiet.
>
> The patch looks good to me.

Applied to master and 14, thanks for review!

--
Daniel Gustafsson        https://vmware.com/

On 20.08.21 12:49, Daniel Gustafsson wrote:
>> On 19 Aug 2021, at 04:45, Julien Rouhaud <rjuju123@gmail.com> wrote:
>>
>> On Thu, Aug 19, 2021 at 2:53 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>>>
>>>> On 18 Aug 2021, at 15:49, Daniel Gustafsson <daniel@yesql.se> wrote:
>>>>
>>>> Since there is consensus for removing --quiet, I’ll propose a patch in a bit
>>>> for removing it and fixing up the tests.
>>>
>>> Turns I was a bit undercaffeinated when skimming the tests, it really wasn’t
>>> that invasive.  The attached diff removes --quiet and fixes up the tests and
>>> docs to match.  Anyone who wants to keep --quiet should..  ehm, not keep quiet.
>>
>> The patch looks good to me.
> 
> Applied to master and 14, thanks for review!

I see that we still have the PQsetErrorVerbosity() call for --verbose, 
and we still issue warnings with --no-strict-names.  Did we want to keep 
these two behaviors?

> On 27 Aug 2021, at 13:21, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>
> On 20.08.21 12:49, Daniel Gustafsson wrote:
>>> On 19 Aug 2021, at 04:45, Julien Rouhaud <rjuju123@gmail.com> wrote:
>>>
>>> On Thu, Aug 19, 2021 at 2:53 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>>>>
>>>>> On 18 Aug 2021, at 15:49, Daniel Gustafsson <daniel@yesql.se> wrote:
>>>>>
>>>>> Since there is consensus for removing --quiet, I’ll propose a patch in a bit
>>>>> for removing it and fixing up the tests.
>>>>
>>>> Turns I was a bit undercaffeinated when skimming the tests, it really wasn’t
>>>> that invasive.  The attached diff removes --quiet and fixes up the tests and
>>>> docs to match.  Anyone who wants to keep --quiet should..  ehm, not keep quiet.
>>>
>>> The patch looks good to me.
>> Applied to master and 14, thanks for review!
>
> I see that we still have the PQsetErrorVerbosity() call for --verbose, and we still issue warnings with
--no-strict-names. Did we want to keep these two behaviors? 

I think issuing the warnings for --no-strict-names is appropriate, setting
PQsetErrorVerbosity on --verbose might be less so although I don’t strong
feelings on that.  Maybe setting that should be reserved for the extra verbose
levels (that many other projects have with -vv etc) if we ever add that?

--
Daniel Gustafsson        https://vmware.com/