Horton

Thread

Commits

  1. pg_verify_checksums: rename -d to --verbose

  1. pg_verify_checksums -r option

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-08-24T16:01:09Z

    I'm curious about this option:

  -r RELFILENODE         check only relation with specified relfilenode

but there is no facility to specify a database.

Also, referring to the relfilenode of a mapped relation seems a bit
inaccurate.

Maybe reframing this in terms of the file name of the file you want
checked would be better?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  2. Re: pg_verify_checksums -r option

    Yugo Nagata <nagata@sraoss.co.jp> — 2018-08-27T10:53:36Z

    On Fri, 24 Aug 2018 18:01:09 +0200
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:

> I'm curious about this option:
> 
>   -r RELFILENODE         check only relation with specified relfilenode
> 
> but there is no facility to specify a database.
> 
> Also, referring to the relfilenode of a mapped relation seems a bit
> inaccurate.
> 
> Maybe reframing this in terms of the file name of the file you want
> checked would be better?

If we specified 1234 to -r option, pg_verify_shceksums checks not only 1234
but also 1234_vm, 1234_fsm, and 1234.1, 1234.2, ... and so on, so I think
it makes senses to allow to specify a relfilenode instead of a file name.

I think it is reasonable to add a option to specify a database, although
I don't know which character is good because both -d and -D are already used....

Regards,
-- 
Yugo Nagata <nagata@sraoss.co.jp>

  3. pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

    Michael Banck <michael.banck@credativ.de> — 2018-08-27T11:34:12Z

    Hi,

On Mon, Aug 27, 2018 at 07:53:36PM +0900, Yugo Nagata wrote:
> On Fri, 24 Aug 2018 18:01:09 +0200
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
> > I'm curious about this option:
> > 
> >   -r RELFILENODE         check only relation with specified relfilenode
> > 
> > but there is no facility to specify a database.
> > 
> > Also, referring to the relfilenode of a mapped relation seems a bit
> > inaccurate.
> > 
> > Maybe reframing this in terms of the file name of the file you want
> > checked would be better?
> 
> If we specified 1234 to -r option, pg_verify_shceksums checks not only 1234
> but also 1234_vm, 1234_fsm, and 1234.1, 1234.2, ... and so on, so I think
> it makes senses to allow to specify a relfilenode instead of a file name.
> 
> I think it is reasonable to add a option to specify a database, although
> I don't know which character is good because both -d and -D are already used....

Maybe the -d (debug) option should be revisited as well. Mentioning
every scanned block generates a huge amount of output which might be
useful during development but does not seem very useful for a stable
release. AFAICT there is no other debug output for now.

So it could be renamed to -v (verbose) and only mention each scanned
file, e.g. (errors/checksum mismatches are still reported of course).

Then -d could (in the future, I guess that is too late for v11) be used
for -d/--dbname (or make that only a long option, if the above does not
work).


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

  4. Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

    Yugo Nagata <nagata@sraoss.co.jp> — 2018-08-27T12:05:33Z

    On Mon, 27 Aug 2018 13:34:12 +0200
Michael Banck <michael.banck@credativ.de> wrote:

> Hi,
> 
> On Mon, Aug 27, 2018 at 07:53:36PM +0900, Yugo Nagata wrote:
> > On Fri, 24 Aug 2018 18:01:09 +0200
> > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
> > > I'm curious about this option:
> > > 
> > >   -r RELFILENODE         check only relation with specified relfilenode
> > > 
> > > but there is no facility to specify a database.
> > > 
> > > Also, referring to the relfilenode of a mapped relation seems a bit
> > > inaccurate.
> > > 
> > > Maybe reframing this in terms of the file name of the file you want
> > > checked would be better?
> > 
> > If we specified 1234 to -r option, pg_verify_shceksums checks not only 1234
> > but also 1234_vm, 1234_fsm, and 1234.1, 1234.2, ... and so on, so I think
> > it makes senses to allow to specify a relfilenode instead of a file name.
> > 
> > I think it is reasonable to add a option to specify a database, although
> > I don't know which character is good because both -d and -D are already used....
> 
> Maybe the -d (debug) option should be revisited as well. Mentioning
> every scanned block generates a huge amount of output which might be
> useful during development but does not seem very useful for a stable
> release. AFAICT there is no other debug output for now.
> 
> So it could be renamed to -v (verbose) and only mention each scanned
> file, e.g. (errors/checksum mismatches are still reported of course).
> 
> Then -d could (in the future, I guess that is too late for v11) be used
> for -d/--dbname (or make that only a long option, if the above does not
> work).

I realized after sending the previous post that we can not specify a database
by name because pg_verify_checksum is run in offline and this can not get the
OID from the database name.  Also, there are global and pg_tblspc directories
not only base/<database OID>. So, it seems to me good to specify a directories
to scan which is under PGDATA. We would be able to use -d ( or --directory ?)
for this purpose.


Regards,
-- 
Yugo Nagata <nagata@sraoss.co.jp>

  5. Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

    Daniel Gustafsson <daniel@yesql.se> — 2018-08-29T08:28:33Z

    > On 27 Aug 2018, at 14:05, Yugo Nagata <nagata@sraoss.co.jp> wrote:
> 
> On Mon, 27 Aug 2018 13:34:12 +0200
> Michael Banck <michael.banck@credativ.de> wrote:
> 
>> Hi,
>> 
>> On Mon, Aug 27, 2018 at 07:53:36PM +0900, Yugo Nagata wrote:
>>> On Fri, 24 Aug 2018 18:01:09 +0200
>>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
>>>> I'm curious about this option:
>>>> 
>>>>  -r RELFILENODE         check only relation with specified relfilenode
>>>> 
>>>> but there is no facility to specify a database.
>>>> 
>>>> Also, referring to the relfilenode of a mapped relation seems a bit
>>>> inaccurate.
>>>> 
>>>> Maybe reframing this in terms of the file name of the file you want
>>>> checked would be better?
>>> 
>>> If we specified 1234 to -r option, pg_verify_shceksums checks not only 1234
>>> but also 1234_vm, 1234_fsm, and 1234.1, 1234.2, ... and so on, so I think
>>> it makes senses to allow to specify a relfilenode instead of a file name.
>>> 
>>> I think it is reasonable to add a option to specify a database, although
>>> I don't know which character is good because both -d and -D are already used....
>> 
>> Maybe the -d (debug) option should be revisited as well. Mentioning
>> every scanned block generates a huge amount of output which might be
>> useful during development but does not seem very useful for a stable
>> release. AFAICT there is no other debug output for now.
>> 
>> So it could be renamed to -v (verbose) and only mention each scanned
>> file, e.g. (errors/checksum mismatches are still reported of course).
>> 
>> Then -d could (in the future, I guess that is too late for v11) be used
>> for -d/--dbname (or make that only a long option, if the above does not
>> work).
> 
> I realized after sending the previous post that we can not specify a database
> by name because pg_verify_checksum is run in offline and this can not get the
> OID from the database name.  Also, there are global and pg_tblspc directories
> not only base/<database OID>. So, it seems to me good to specify a directories
> to scan which is under PGDATA. We would be able to use -d ( or --directory ?)
> for this purpose.

Changing functionality to the above discussed is obviously 12 material, but
since we are discussing changing the command line API of the tool by
repurposing -d; do we want to rename the current use of -d to -v (with the
accompanying —-verbose) before 11 ships?  It’s clearly way way too late in the
cycle but it seems worth to at least bring up since 11 will be the first
version pg_verify_checksums ship in. I’m happy to do the work asap if so.

FWIW, personally I think verbose makes more sense for the output than debug.

cheers ./daniel

  6. Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

    Yugo Nagata <nagata@sraoss.co.jp> — 2018-08-29T11:33:43Z

    On Wed, 29 Aug 2018 10:28:33 +0200
Daniel Gustafsson <daniel@yesql.se> wrote:

> > On 27 Aug 2018, at 14:05, Yugo Nagata <nagata@sraoss.co.jp> wrote:
> > 
> > On Mon, 27 Aug 2018 13:34:12 +0200
> > Michael Banck <michael.banck@credativ.de> wrote:
> > 
> >> Hi,
> >> 
> >> On Mon, Aug 27, 2018 at 07:53:36PM +0900, Yugo Nagata wrote:
> >>> On Fri, 24 Aug 2018 18:01:09 +0200
> >>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
> >>>> I'm curious about this option:
> >>>> 
> >>>>  -r RELFILENODE         check only relation with specified relfilenode
> >>>> 
> >>>> but there is no facility to specify a database.
> >>>> 
> >>>> Also, referring to the relfilenode of a mapped relation seems a bit
> >>>> inaccurate.
> >>>> 
> >>>> Maybe reframing this in terms of the file name of the file you want
> >>>> checked would be better?
> >>> 
> >>> If we specified 1234 to -r option, pg_verify_shceksums checks not only 1234
> >>> but also 1234_vm, 1234_fsm, and 1234.1, 1234.2, ... and so on, so I think
> >>> it makes senses to allow to specify a relfilenode instead of a file name.
> >>> 
> >>> I think it is reasonable to add a option to specify a database, although
> >>> I don't know which character is good because both -d and -D are already used....
> >> 
> >> Maybe the -d (debug) option should be revisited as well. Mentioning
> >> every scanned block generates a huge amount of output which might be
> >> useful during development but does not seem very useful for a stable
> >> release. AFAICT there is no other debug output for now.
> >> 
> >> So it could be renamed to -v (verbose) and only mention each scanned
> >> file, e.g. (errors/checksum mismatches are still reported of course).
> >> 
> >> Then -d could (in the future, I guess that is too late for v11) be used
> >> for -d/--dbname (or make that only a long option, if the above does not
> >> work).
> > 
> > I realized after sending the previous post that we can not specify a database
> > by name because pg_verify_checksum is run in offline and this can not get the
> > OID from the database name.  Also, there are global and pg_tblspc directories
> > not only base/<database OID>. So, it seems to me good to specify a directories
> > to scan which is under PGDATA. We would be able to use -d ( or --directory ?)
> > for this purpose.
> 
> Changing functionality to the above discussed is obviously 12 material, but
> since we are discussing changing the command line API of the tool by
> repurposing -d; do we want to rename the current use of -d to -v (with the
> accompanying ―-verbose) before 11 ships?  It’s clearly way way too late in the
> cycle but it seems worth to at least bring up since 11 will be the first
> version pg_verify_checksums ship in. I’m happy to do the work asap if so.

I agree with this.  Almost other commands doesn't use -d as debug mode
although there a few exception, and instead -v is used for verbose mode.
If we can change the command line of pg_veriry_checksums, before release of
PG 11 is best.  Attached is the patch to do this.

Regards,
-- 
Yugo Nagata <nagata@sraoss.co.jp>

  7. Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

    Michael Banck <michael.banck@credativ.de> — 2018-08-29T11:37:49Z

    Hi,

On Wed, Aug 29, 2018 at 08:33:43PM +0900, Yugo Nagata wrote:
> On Wed, 29 Aug 2018 10:28:33 +0200
> Daniel Gustafsson <daniel@yesql.se> wrote:
> > > On 27 Aug 2018, at 14:05, Yugo Nagata <nagata@sraoss.co.jp> wrote:
> > > On Mon, 27 Aug 2018 13:34:12 +0200
> > > Michael Banck <michael.banck@credativ.de> wrote:
> > >> On Mon, Aug 27, 2018 at 07:53:36PM +0900, Yugo Nagata wrote:
> > >>> On Fri, 24 Aug 2018 18:01:09 +0200
> > >>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
> > >>>> I'm curious about this option:
> > >>>> 
> > >>>>  -r RELFILENODE         check only relation with specified relfilenode
> > >>>> 
> > >>>> but there is no facility to specify a database.
> > >>>> 
> > >>>> Also, referring to the relfilenode of a mapped relation seems a bit
> > >>>> inaccurate.
> > >>>> 
> > >>>> Maybe reframing this in terms of the file name of the file you want
> > >>>> checked would be better?
> > >>> 
> > >>> If we specified 1234 to -r option, pg_verify_shceksums checks not only 1234
> > >>> but also 1234_vm, 1234_fsm, and 1234.1, 1234.2, ... and so on, so I think
> > >>> it makes senses to allow to specify a relfilenode instead of a file name.
> > >>> 
> > >>> I think it is reasonable to add a option to specify a database, although
> > >>> I don't know which character is good because both -d and -D are already used....
> > >> 
> > >> Maybe the -d (debug) option should be revisited as well. Mentioning
> > >> every scanned block generates a huge amount of output which might be
> > >> useful during development but does not seem very useful for a stable
> > >> release. AFAICT there is no other debug output for now.
> > >> 
> > >> So it could be renamed to -v (verbose) and only mention each scanned
> > >> file, e.g. (errors/checksum mismatches are still reported of course).

I still think this should be changed as well, i.e. -v should not report
every block scanned, as that really is debug output and IMO not useful
in general? AFAICT your page does not change the output at all, just
renames the option.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

  8. Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

    Magnus Hagander <magnus@hagander.net> — 2018-08-29T11:46:38Z

    On Wed, Aug 29, 2018 at 1:37 PM, Michael Banck <michael.banck@credativ.de>
wrote:

> Hi,
>
> On Wed, Aug 29, 2018 at 08:33:43PM +0900, Yugo Nagata wrote:
> > On Wed, 29 Aug 2018 10:28:33 +0200
> > Daniel Gustafsson <daniel@yesql.se> wrote:
> > > > On 27 Aug 2018, at 14:05, Yugo Nagata <nagata@sraoss.co.jp> wrote:
> > > > On Mon, 27 Aug 2018 13:34:12 +0200
> > > > Michael Banck <michael.banck@credativ.de> wrote:
> > > >> On Mon, Aug 27, 2018 at 07:53:36PM +0900, Yugo Nagata wrote:
> > > >>> On Fri, 24 Aug 2018 18:01:09 +0200
> > > >>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
> > > >>>> I'm curious about this option:
> > > >>>>
> > > >>>>  -r RELFILENODE         check only relation with specified
> relfilenode
> > > >>>>
> > > >>>> but there is no facility to specify a database.
> > > >>>>
> > > >>>> Also, referring to the relfilenode of a mapped relation seems a
> bit
> > > >>>> inaccurate.
> > > >>>>
> > > >>>> Maybe reframing this in terms of the file name of the file you
> want
> > > >>>> checked would be better?
> > > >>>
> > > >>> If we specified 1234 to -r option, pg_verify_shceksums checks not
> only 1234
> > > >>> but also 1234_vm, 1234_fsm, and 1234.1, 1234.2, ... and so on, so
> I think
> > > >>> it makes senses to allow to specify a relfilenode instead of a
> file name.
> > > >>>
> > > >>> I think it is reasonable to add a option to specify a database,
> although
> > > >>> I don't know which character is good because both -d and -D are
> already used....
> > > >>
> > > >> Maybe the -d (debug) option should be revisited as well. Mentioning
> > > >> every scanned block generates a huge amount of output which might be
> > > >> useful during development but does not seem very useful for a stable
> > > >> release. AFAICT there is no other debug output for now.
> > > >>
> > > >> So it could be renamed to -v (verbose) and only mention each scanned
> > > >> file, e.g. (errors/checksum mismatches are still reported of
> course).
>
> I still think this should be changed as well, i.e. -v should not report
> every block scanned, as that really is debug output and IMO not useful
> in general? AFAICT your page does not change the output at all, just
> renames the option.
>
>
I agree with this (though it's my fault initially :P). Per-page output is
going to be useless in pretty much all production cases. It makes sense to
also change it to be per-file.

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

  9. Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

    Yugo Nagata <nagata@sraoss.co.jp> — 2018-08-29T12:09:03Z

    On Wed, 29 Aug 2018 13:46:38 +0200
Magnus Hagander <magnus@hagander.net> wrote:

> On Wed, Aug 29, 2018 at 1:37 PM, Michael Banck <michael.banck@credativ.de>
> wrote:
> 
> > Hi,
> >
> > On Wed, Aug 29, 2018 at 08:33:43PM +0900, Yugo Nagata wrote:
> > > On Wed, 29 Aug 2018 10:28:33 +0200
> > > Daniel Gustafsson <daniel@yesql.se> wrote:
> > > > > On 27 Aug 2018, at 14:05, Yugo Nagata <nagata@sraoss.co.jp> wrote:
> > > > > On Mon, 27 Aug 2018 13:34:12 +0200
> > > > > Michael Banck <michael.banck@credativ.de> wrote:
> > > > >> On Mon, Aug 27, 2018 at 07:53:36PM +0900, Yugo Nagata wrote:
> > > > >>> On Fri, 24 Aug 2018 18:01:09 +0200
> > > > >>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
> > > > >>>> I'm curious about this option:
> > > > >>>>
> > > > >>>>  -r RELFILENODE         check only relation with specified
> > relfilenode
> > > > >>>>
> > > > >>>> but there is no facility to specify a database.
> > > > >>>>
> > > > >>>> Also, referring to the relfilenode of a mapped relation seems a
> > bit
> > > > >>>> inaccurate.
> > > > >>>>
> > > > >>>> Maybe reframing this in terms of the file name of the file you
> > want
> > > > >>>> checked would be better?
> > > > >>>
> > > > >>> If we specified 1234 to -r option, pg_verify_shceksums checks not
> > only 1234
> > > > >>> but also 1234_vm, 1234_fsm, and 1234.1, 1234.2, ... and so on, so
> > I think
> > > > >>> it makes senses to allow to specify a relfilenode instead of a
> > file name.
> > > > >>>
> > > > >>> I think it is reasonable to add a option to specify a database,
> > although
> > > > >>> I don't know which character is good because both -d and -D are
> > already used....
> > > > >>
> > > > >> Maybe the -d (debug) option should be revisited as well. Mentioning
> > > > >> every scanned block generates a huge amount of output which might be
> > > > >> useful during development but does not seem very useful for a stable
> > > > >> release. AFAICT there is no other debug output for now.
> > > > >>
> > > > >> So it could be renamed to -v (verbose) and only mention each scanned
> > > > >> file, e.g. (errors/checksum mismatches are still reported of
> > course).
> >
> > I still think this should be changed as well, i.e. -v should not report
> > every block scanned, as that really is debug output and IMO not useful
> > in general? AFAICT your page does not change the output at all, just
> > renames the option.
> >
> >
> I agree with this (though it's my fault initially :P). Per-page output is
> going to be useless in pretty much all production cases. It makes sense to
> also change it to be per-file.

I updated the patch to output only per-file information in the verbose mode.
Does this behavior match you expect?

Regards,
-- 
Yugo Nagata <nagata@sraoss.co.jp>

  10. Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

    Yugo Nagata <nagata@sraoss.co.jp> — 2018-08-29T14:25:28Z

    On Wed, 29 Aug 2018 21:09:03 +0900
Yugo Nagata <nagata@sraoss.co.jp> wrote:

> On Wed, 29 Aug 2018 13:46:38 +0200
> Magnus Hagander <magnus@hagander.net> wrote:
> 
> > On Wed, Aug 29, 2018 at 1:37 PM, Michael Banck <michael.banck@credativ.de>
> > wrote:
> > 
> > > Hi,
> > >
> > > On Wed, Aug 29, 2018 at 08:33:43PM +0900, Yugo Nagata wrote:
> > > > On Wed, 29 Aug 2018 10:28:33 +0200
> > > > Daniel Gustafsson <daniel@yesql.se> wrote:
> > > > > > On 27 Aug 2018, at 14:05, Yugo Nagata <nagata@sraoss.co.jp> wrote:
> > > > > > On Mon, 27 Aug 2018 13:34:12 +0200
> > > > > > Michael Banck <michael.banck@credativ.de> wrote:
> > > > > >> On Mon, Aug 27, 2018 at 07:53:36PM +0900, Yugo Nagata wrote:
> > > > > >>> On Fri, 24 Aug 2018 18:01:09 +0200
> > > > > >>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
> > > > > >>>> I'm curious about this option:
> > > > > >>>>
> > > > > >>>>  -r RELFILENODE         check only relation with specified
> > > relfilenode
> > > > > >>>>
> > > > > >>>> but there is no facility to specify a database.
> > > > > >>>>
> > > > > >>>> Also, referring to the relfilenode of a mapped relation seems a
> > > bit
> > > > > >>>> inaccurate.
> > > > > >>>>
> > > > > >>>> Maybe reframing this in terms of the file name of the file you
> > > want
> > > > > >>>> checked would be better?
> > > > > >>>
> > > > > >>> If we specified 1234 to -r option, pg_verify_shceksums checks not
> > > only 1234
> > > > > >>> but also 1234_vm, 1234_fsm, and 1234.1, 1234.2, ... and so on, so
> > > I think
> > > > > >>> it makes senses to allow to specify a relfilenode instead of a
> > > file name.
> > > > > >>>
> > > > > >>> I think it is reasonable to add a option to specify a database,
> > > although
> > > > > >>> I don't know which character is good because both -d and -D are
> > > already used....
> > > > > >>
> > > > > >> Maybe the -d (debug) option should be revisited as well. Mentioning
> > > > > >> every scanned block generates a huge amount of output which might be
> > > > > >> useful during development but does not seem very useful for a stable
> > > > > >> release. AFAICT there is no other debug output for now.
> > > > > >>
> > > > > >> So it could be renamed to -v (verbose) and only mention each scanned
> > > > > >> file, e.g. (errors/checksum mismatches are still reported of
> > > course).
> > >
> > > I still think this should be changed as well, i.e. -v should not report
> > > every block scanned, as that really is debug output and IMO not useful
> > > in general? AFAICT your page does not change the output at all, just
> > > renames the option.
> > >
> > >
> > I agree with this (though it's my fault initially :P). Per-page output is
> > going to be useless in pretty much all production cases. It makes sense to
> > also change it to be per-file.
> 
> I updated the patch to output only per-file information in the verbose mode.
> Does this behavior match you expect?

I am sorry but I attached a wrong file in the previous post.
Attached is the correct version of the updated patch.

Regards,
-- 
Yugo Nagata <nagata@sraoss.co.jp>

  11. Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

    Michael Banck <michael.banck@credativ.de> — 2018-08-30T08:13:31Z

    Hi,

thanks for fixing this up!

On Wed, Aug 29, 2018 at 11:25:28PM +0900, Yugo Nagata wrote:
> diff --git a/doc/src/sgml/ref/pg_verify_checksums.sgml b/doc/src/sgml/ref/pg_verify_checksums.sgml
> index ecc5501eae..a1ff060c2b 100644
> --- a/doc/src/sgml/ref/pg_verify_checksums.sgml
> +++ b/doc/src/sgml/ref/pg_verify_checksums.sgml
> @@ -64,8 +64,8 @@ PostgreSQL documentation
>        <term><option>-d</option></term>

This -d needs to be renamed to -v as well I guess.

>        <listitem>
>         <para>
> -        Enable debug output. Lists all checked blocks and their checksum.
> -       </para>
> +        Enable debug output. Lists all checked files.

I'd say 'Enable verbose output.' would be more appropriate now.

Looks good to me otherwise.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

  12. Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

    Michael Banck <michael.banck@credativ.de> — 2018-08-30T08:34:06Z

    Hi,

On Thu, Aug 30, 2018 at 05:35:09PM +0900, Yugo Nagata wrote:
> --- a/doc/src/sgml/ref/pg_verify_checksums.sgml
> +++ b/doc/src/sgml/ref/pg_verify_checksums.sgml
> @@ -61,11 +61,11 @@ PostgreSQL documentation
>       </varlistentry>
>  
>       <varlistentry>
> -      <term><option>-d</option></term>
> +      <term><option>-v</option></term>

Sorry that I did not catch this the first time, but as you have added
the --verbose long option, I think that should be documented here as
well, similar to -D/--pgdata.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

  13. Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

    Yugo Nagata <nagata@sraoss.co.jp> — 2018-08-30T08:35:09Z

    On Thu, 30 Aug 2018 10:13:31 +0200
Michael Banck <michael.banck@credativ.de> wrote:

> Hi,
> 
> thanks for fixing this up!
> 
> On Wed, Aug 29, 2018 at 11:25:28PM +0900, Yugo Nagata wrote:
> > diff --git a/doc/src/sgml/ref/pg_verify_checksums.sgml b/doc/src/sgml/ref/pg_verify_checksums.sgml
> > index ecc5501eae..a1ff060c2b 100644
> > --- a/doc/src/sgml/ref/pg_verify_checksums.sgml
> > +++ b/doc/src/sgml/ref/pg_verify_checksums.sgml
> > @@ -64,8 +64,8 @@ PostgreSQL documentation
> >        <term><option>-d</option></term>
> 
> This -d needs to be renamed to -v as well I guess.
> 
> >        <listitem>
> >         <para>
> > -        Enable debug output. Lists all checked blocks and their checksum.
> > -       </para>
> > +        Enable debug output. Lists all checked files.
> 
> I'd say 'Enable verbose output.' would be more appropriate now.
> 
> Looks good to me otherwise.

Thank you for your review. I forgot the doc fix.
Attached is the updated patch.

Regards,
-- 
Yugo Nagata <nagata@sraoss.co.jp>

  14. Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

    Yugo Nagata <nagata@sraoss.co.jp> — 2018-08-30T08:48:24Z

    On Thu, 30 Aug 2018 10:34:06 +0200
Michael Banck <michael.banck@credativ.de> wrote:

> Hi,
> 
> On Thu, Aug 30, 2018 at 05:35:09PM +0900, Yugo Nagata wrote:
> > --- a/doc/src/sgml/ref/pg_verify_checksums.sgml
> > +++ b/doc/src/sgml/ref/pg_verify_checksums.sgml
> > @@ -61,11 +61,11 @@ PostgreSQL documentation
> >       </varlistentry>
> >  
> >       <varlistentry>
> > -      <term><option>-d</option></term>
> > +      <term><option>-v</option></term>
> 
> Sorry that I did not catch this the first time, but as you have added
> the --verbose long option, I think that should be documented here as
> well, similar to -D/--pgdata.

Oops, It's my mistake. I updated the patch.

Thanks,

> 
> 
> Michael
> 
> -- 
> Michael Banck
> Projektleiter / Senior Berater
> Tel.: +49 2166 9901-171
> Fax:  +49 2166 9901-100
> Email: michael.banck@credativ.de
> 
> credativ GmbH, HRB Mönchengladbach 12080
> USt-ID-Nummer: DE204566209
> Trompeterallee 108, 41189 Mönchengladbach
> Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
> 
> Unser Umgang mit personenbezogenen Daten unterliegt
> folgenden Bestimmungen: https://www.credativ.de/datenschutz


-- 
Yugo Nagata <nagata@sraoss.co.jp>

  15. Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

    Michael Banck <michael.banck@credativ.de> — 2018-08-30T08:54:08Z

    Hi,

On Thu, Aug 30, 2018 at 05:48:24PM +0900, Yugo Nagata wrote:
> Oops, It's my mistake. I updated the patch.

Looks good to me now.

One could argue that the message could be 'checksums verified in file
FILE' (plural) rather than 'checksum verified in file FILE', but that is
quickly approaching bikeshed territory I guess.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

  16. Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

    Yugo Nagata <nagata@sraoss.co.jp> — 2018-08-30T09:10:54Z

    On Thu, 30 Aug 2018 10:54:08 +0200
Michael Banck <michael.banck@credativ.de> wrote:

> Hi,
> 
> On Thu, Aug 30, 2018 at 05:48:24PM +0900, Yugo Nagata wrote:
> > Oops, It's my mistake. I updated the patch.
> 
> Looks good to me now.
> 
> One could argue that the message could be 'checksums verified in file
> FILE' (plural) rather than 'checksum verified in file FILE', but that is
> quickly approaching bikeshed territory I guess.

It seems to me reasonable. Although I'm not native English speaker, if this
is more natural for Westerners, it is better to fix. 

Attached is the revised version.

Regards,
-- 
Yugo Nagata <nagata@sraoss.co.jp>

  17. Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-08-30T09:52:58Z

    Thanks! Pushed.  There was a markup error in the documentation.


This should have been listed in the pg11 open items.  Please list there
everything that should apply be applied branch 11 before release, so
that they get fixed (or at least considered) before we release.

https://wiki.postgresql.org/wiki/Open_Items

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  18. Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

    Yugo Nagata <nagata@sraoss.co.jp> — 2018-08-30T10:14:43Z

    On Thu, 30 Aug 2018 06:52:58 -0300
Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

> Thanks! Pushed.  There was a markup error in the documentation.

Thank you!

> 
> 
> This should have been listed in the pg11 open items.  Please list there
> everything that should apply be applied branch 11 before release, so
> that they get fixed (or at least considered) before we release.
> 
> https://wiki.postgresql.org/wiki/Open_Items

I don't have the editor privilege now, so I'll add this discussion to the 
wiki (Fixed issues or Resolve issues?) after I get the privilege.

Regards,
-- 
Yugo Nagata <nagata@sraoss.co.jp>

  19. Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-08-30T10:18:13Z

    On 2018-Aug-30, Yugo Nagata wrote:

> On Thu, 30 Aug 2018 06:52:58 -0300
> Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

> > This should have been listed in the pg11 open items.  Please list there
> > everything that should apply be applied branch 11 before release, so
> > that they get fixed (or at least considered) before we release.
> > 
> > https://wiki.postgresql.org/wiki/Open_Items
> 
> I don't have the editor privilege now, so I'll add this discussion to the 
> wiki (Fixed issues or Resolve issues?) after I get the privilege.

You're an editor now, though IMO adding it as a resolved issue is
pointless.  I meant if there are any items pending resolution, then
please add them.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  20. Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

    Yugo Nagata <nagata@sraoss.co.jp> — 2018-08-30T11:19:14Z

    On Thu, 30 Aug 2018 07:18:13 -0300
Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

> On 2018-Aug-30, Yugo Nagata wrote:
> 
> > On Thu, 30 Aug 2018 06:52:58 -0300
> > Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> 
> > > This should have been listed in the pg11 open items.  Please list there
> > > everything that should apply be applied branch 11 before release, so
> > > that they get fixed (or at least considered) before we release.
> > > 
> > > https://wiki.postgresql.org/wiki/Open_Items
> > 
> > I don't have the editor privilege now, so I'll add this discussion to the 
> > wiki (Fixed issues or Resolve issues?) after I get the privilege.
> 
> You're an editor now, though IMO adding it as a resolved issue is
> pointless.  I meant if there are any items pending resolution, then
> please add them.

Sure. I will do that from next time.

Regards,
-- 
Yugo Nagata <nagata@sraoss.co.jp>

  21. Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

    Yugo Nagata <nagata@sraoss.co.jp> — 2018-08-31T10:52:43Z

    On Mon, 27 Aug 2018 21:05:33 +0900
Yugo Nagata <nagata@sraoss.co.jp> wrote:

> On Mon, 27 Aug 2018 13:34:12 +0200
> Michael Banck <michael.banck@credativ.de> wrote:
> 
> > Hi,
> > 
> > On Mon, Aug 27, 2018 at 07:53:36PM +0900, Yugo Nagata wrote:
> > > On Fri, 24 Aug 2018 18:01:09 +0200
> > > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
> > > > I'm curious about this option:
> > > > 
> > > >   -r RELFILENODE         check only relation with specified relfilenode
> > > > 
> > > > but there is no facility to specify a database.
> > > > 
> > > > Also, referring to the relfilenode of a mapped relation seems a bit
> > > > inaccurate.
> > > > 
> > > > Maybe reframing this in terms of the file name of the file you want
> > > > checked would be better?
> > > 
> > > If we specified 1234 to -r option, pg_verify_shceksums checks not only 1234
> > > but also 1234_vm, 1234_fsm, and 1234.1, 1234.2, ... and so on, so I think
> > > it makes senses to allow to specify a relfilenode instead of a file name.
> > > 
> > > I think it is reasonable to add a option to specify a database, although
> > > I don't know which character is good because both -d and -D are already used....
> > 
> > Maybe the -d (debug) option should be revisited as well. Mentioning
> > every scanned block generates a huge amount of output which might be
> > useful during development but does not seem very useful for a stable
> > release. AFAICT there is no other debug output for now.
> > 
> > So it could be renamed to -v (verbose) and only mention each scanned
> > file, e.g. (errors/checksum mismatches are still reported of course).
> > 
> > Then -d could (in the future, I guess that is too late for v11) be used
> > for -d/--dbname (or make that only a long option, if the above does not
> > work).
> 
> I realized after sending the previous post that we can not specify a database
> by name because pg_verify_checksum is run in offline and this can not get the
> OID from the database name.  Also, there are global and pg_tblspc directories
> not only base/<database OID>. So, it seems to me good to specify a directories
> to scan which is under PGDATA. We would be able to use -d ( or --directory ?)
> for this purpose.

Attached is a patch to allow pg_verity_checksums to specify a database
to scan.  This is usefule for users who want to verify checksums of relations
in a specific database. We can specify a database by OID using -d or --dboid option.
Also, when -g or --global-only is used only shared relations are scaned.

Regards,
-- 
Yugo Nagata <nagata@sraoss.co.jp>

  22. Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

    Fabien COELHO <coelho@cri.ensmp.fr> — 2018-09-01T05:40:40Z

    Hello Yugo-san,

> Attached is a patch to allow pg_verity_checksums to specify a database
> to scan.  This is usefule for users who want to verify checksums of relations
> in a specific database. We can specify a database by OID using -d or --dboid option.
> Also, when -g or --global-only is used only shared relations are scaned.

It seems that the patch does not apply anymore. Could you rebase it?

-- 
Fabien.

  23. Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

    Yugo Nagata <nagata@sraoss.co.jp> — 2018-09-03T13:06:18Z

    Hi,

On Sat, 1 Sep 2018 07:40:40 +0200 (CEST)
Fabien COELHO <coelho@cri.ensmp.fr> wrote:

> > Attached is a patch to allow pg_verity_checksums to specify a database
> > to scan.  This is usefule for users who want to verify checksums of relations
> > in a specific database. We can specify a database by OID using -d or --dboid option.
> > Also, when -g or --global-only is used only shared relations are scaned.
> 
> It seems that the patch does not apply anymore. Could you rebase it?

I attached the rebased patch.

Regards,
-- 
Yugo Nagata <nagata@sraoss.co.jp>

  24. Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

    Fabien COELHO <coelho@cri.ensmp.fr> — 2018-09-03T17:14:41Z

    Hello Yugo-san,

> I attached the rebased patch.

Patch applies cleanly, compiles, "make check" is okay, although there are 
no specific test for the feature. Indeed, after investigation there is not 
a SINGLE test for the command:-(

I think that some minimal tap-testing should be done. It seems that 
pg_basebackup tap test is the only one which enables checksums. Maybe a 
pg_verify_checksum could be added to the "010_pg_basebackup.pl" script.

Anyway I tested that it works by hex-editing a file to trigger a fail.

Function "atoi" is quite lazy, it accepts "-d 1zzz" as "1". Maybe parsing 
could be stricter.

When the command is started with both "-d 1 -g", it succeeds by checking 
nothing, which is quite misleading. Probably it should complain that these 
options are mutually exclusive, or it should check both under -g AND -d 1?

The oid of a database is not obvious... You have to query "SELECT oid, *", 
it is not given by \l or \l+ or "psql -l".

About the documentation:

"Only validate checksums in the relations in the database with specified 
OID."... I think that indexes and other possibly toasted values are also 
checked. I'd suggest "Only validate checksums of objects in the database 
specified by its OID".

"--globel-only" -> "--global-only".

ISTM that --help should show options in alphabetical order, however -v is 
out of order.

-- 
Fabien.

  25. Re: pg_verify_checksums -d option (was: Re: pg_verify_checksums -r option)

    Masahiko Sawada <sawada.mshk@gmail.com> — 2018-09-04T08:16:09Z

    On Mon, Sep 3, 2018 at 10:06 PM, Yugo Nagata <nagata@sraoss.co.jp> wrote:
> Hi,
>
> On Sat, 1 Sep 2018 07:40:40 +0200 (CEST)
> Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>
>> > Attached is a patch to allow pg_verity_checksums to specify a database
>> > to scan.  This is usefule for users who want to verify checksums of relations
>> > in a specific database. We can specify a database by OID using -d or --dboid option.
>> > Also, when -g or --global-only is used only shared relations are scaned.
>>
>> It seems that the patch does not apply anymore. Could you rebase it?
>
> I attached the rebased patch.

FWIW, I think it would be good if we can specify multiple database
oids to the -d option, like "pg_verify_checksums -d 12345 -d 23456".
Maybe it's the same for the -r option.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center