pg_verify_checksums -r option

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>

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

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>

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

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>

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

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/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

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>

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>

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

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

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>

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>

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

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>

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

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>

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

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>

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>

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.

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>

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.

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