Thread

Commits

Remove --quiet option from pg_amcheck
- 9a9c8b92018d 15.0 landed
- 92ce7f527960 14.0 landed

BUG #17148: About --no-strict-names option and --quiet option of pg_amcheck command

The Post Office <noreply@postgresql.org> — 2021-08-17T04:34:28Z

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.

Re: BUG #17148: About --no-strict-names option and --quiet option of pg_amcheck command

Daniel Gustafsson <daniel@yesql.se> — 2021-08-17T08:14:18Z

> 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/

Re: BUG #17148: About --no-strict-names option and --quiet option of pg_amcheck command

Daniel Gustafsson <daniel@yesql.se> — 2021-08-17T11:06:12Z

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/

Re: BUG #17148: About --no-strict-names option and --quiet option of pg_amcheck command

Euler Taveira <euler@eulerto.com> — 2021-08-17T14:53:07Z

On Tue, Aug 17, 2021, at 8:06 AM, Daniel Gustafsson wrote:
> 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:
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". This message is probably
more accurate than "don't write any messages" even for the other binaries
(clusterdb, reindexdb, and vacuumdb) too.

For example, vacuumdb prints an error message even though --quiet is provided.

$ vacuumdb -d postgres -p 8888 --quiet
vacuumdb: error: connection to server on socket "/tmp/.s.PGSQL.8888" failed: No such file or directory
Is the server running locally and accepting connections on that socket?

quiet option is not strict; it doesn't omit *all* messages (including fatal or
error messages). We can debate if the current behavior is fine for most use
cases. It might surprise users while writing a script that they should discard
the standard error output even though the command contains a --quiet option.

That's a different (but related) patch but IMO we should change the --help
description because it is (a) not accurate and (b) doesn't match the
documentation: "don't write any messages" vs "Do not display progress
messages".

--
Euler Taveira
EDB   https://www.enterprisedb.com/

Re: BUG #17148: About --no-strict-names option and --quiet option of pg_amcheck command

Daniel Gustafsson <daniel@yesql.se> — 2021-08-17T15:57:36Z

> 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/

Re: BUG #17148: About --no-strict-names option and --quiet option of pg_amcheck command

Euler Taveira <euler@eulerto.com> — 2021-08-17T17:00:15Z


On Tue, Aug 17, 2021, at 12:57 PM, Daniel Gustafsson wrote:
> > 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.
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.


--
Euler Taveira
EDB   https://www.enterprisedb.com/

Re: BUG #17148: About --no-strict-names option and --quiet option of pg_amcheck command

Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-08-18T11:44:00Z

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.

Re: BUG #17148: About --no-strict-names option and --quiet option of pg_amcheck command

Daniel Gustafsson <daniel@yesql.se> — 2021-08-18T12:35:06Z

> 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/

Re: BUG #17148: About --no-strict-names option and --quiet option of pg_amcheck command

Tom Lane <tgl@sss.pgh.pa.us> — 2021-08-18T13:42:50Z

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

Re: BUG #17148: About --no-strict-names option and --quiet option of pg_amcheck command

Magnus Hagander <magnus@hagander.net> — 2021-08-18T13:46:20Z

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/

Re: BUG #17148: About --no-strict-names option and --quiet option of pg_amcheck command

Daniel Gustafsson <daniel@yesql.se> — 2021-08-18T13:49:48Z

> 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/

Re: BUG #17148: About --no-strict-names option and --quiet option of pg_amcheck command

Daniel Gustafsson <daniel@yesql.se> — 2021-08-18T18:53:00Z

> 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/

Re: BUG #17148: About --no-strict-names option and --quiet option of pg_amcheck command

Julien Rouhaud <rjuju123@gmail.com> — 2021-08-19T02:45:50Z

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.

Re: BUG #17148: About --no-strict-names option and --quiet option of pg_amcheck command

Daniel Gustafsson <daniel@yesql.se> — 2021-08-20T10:49:57Z

> 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/

Re: BUG #17148: About --no-strict-names option and --quiet option of pg_amcheck command

Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-08-27T11:21:52Z

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?

Re: BUG #17148: About --no-strict-names option and --quiet option of pg_amcheck command

Daniel Gustafsson <daniel@yesql.se> — 2021-08-27T12:02:10Z

> 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/