From: Justin Pryzby <pryzby@telsasoft.com>
To: Julien Rouhaud <rjuju123@gmail.com>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org, Masahiko Sawada <sawada.mshk@gmail.com>
Subject: Re: Online checksums verification in the backend
Date: Sun, 12 Jul 2020 12:34:03 -0500
Message-ID: <20200712173403.GI26220@telsasoft.com> (raw)
In-Reply-To: <20200405094459.GG1206@nol>
References: <20200318101055.GA36918@nol>
<CA+fd4k4B8QVxhQa8=5pf7SuDcB0vBhqzOn0c59FxVa99CF0ptQ@mail.gmail.com>
<20200328121858.GA13605@nol>
<CA+fd4k4--o41NxwN2kjfRn6W474R-P9D8aL8FW27nQmWm0++hA@mail.gmail.com>
<20200403093911.GB1206@nol>
<20200404090428.GD1206@nol>
<CA+fd4k7c83ViCvZtochX9msnyigDjcNujphFSXdnd7QOaWXSqA@mail.gmail.com>
<20200405084355.GF1206@nol>
<CA+fd4k5VS_QAkdGwU-mwsr1z78XM7egixZ2bDhfeSJm4HCrRGg@mail.gmail.com>
<20200405094459.GG1206@nol>
Small language fixes in comments and user-facing docs.
inline patch 1: doc/src/sgml/func.sgml, src/backend/storage/page/checksum.c, src/backend/utils/adt/checksumfuncs.c and 2 more (+27/-27 lines) download inline-1.patchdiff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgmlindex 88efb38556..39596db193 100644--- a/doc/src/sgml/func.sgml+++ b/doc/src/sgml/func.sgml@@ -26162,7 +26162,7 @@ SELECT convert_from(pg_read_binary_file('file_in_utf8.txt'), 'UTF8');
<para>
The functions shown in <xref linkend="functions-data-sanity-table"/>
- provide means to check for health of data file in a cluster.+ provide a means to check for health of a data file in a cluster.
</para>
<table id="functions-data-sanity-table">
@@ -26179,8 +26179,8 @@ SELECT convert_from(pg_read_binary_file('file_in_utf8.txt'), 'UTF8');
<literal><function>pg_check_relation(<parameter>relation</parameter> <type>regclass</type> [, <parameter>fork</parameter> <type>text</type>])</function></literal>
</entry>
<entry><type>setof record</type></entry>
- <entry>Validate the checksums for all blocks of all or the given fork of- a given relation.</entry>+ <entry>Validate the checksum for all blocks of a relation.+ </entry>
</row>
</tbody>
</tgroup>
@@ -26190,15 +26190,15 @@ SELECT convert_from(pg_read_binary_file('file_in_utf8.txt'), 'UTF8');
<primary>pg_check_relation</primary>
</indexterm>
<para id="functions-check-relation-note" xreflabel="pg_check_relation">
- <function>pg_check_relation</function> iterates over all the blocks of a- given relation and verify their checksum. If provided,- <replaceable>fork</replaceable> should be <literal>'main'</literal> for the+ <function>pg_check_relation</function> iterates over all blocks of a+ given relation and verifies their checksums. If passed,+ <replaceable>fork</replaceable> specifies that only checksums of the given+ fork are to be verified. Fork should be <literal>'main'</literal> for the
main data fork, <literal>'fsm'</literal> for the free space map,
<literal>'vm'</literal> for the visibility map, or
- <literal>'init'</literal> for the initialization fork, and only this- specific fork will be verifies, otherwise all forks will. The function- returns the list of blocks for which the found checksum doesn't match the- expected one. See <xref+ <literal>'init'</literal> for the initialization fork.+ The function returns a list of blocks for which the computed and stored+ checksums don't match. See <xref
linkend="runtime-config-resource-checksum-verification-cost"/> for
information on how to configure cost-based verification delay. You must be
a member of the <literal>pg_read_all_stats</literal> role to use this
diff --git a/src/backend/storage/page/checksum.c b/src/backend/storage/page/checksum.cindex eb2c919c34..17cd95ec95 100644--- a/src/backend/storage/page/checksum.c+++ b/src/backend/storage/page/checksum.c@@ -36,7 +36,7 @@
* actual storage, you have to discard the operating system cache before
* running those functions.
*
- * To avoid torn page and possible false positive when reading data, and+ * To avoid torn pages and possible false positives when reading data, and to
* keeping overhead as low as possible, the following heuristics are used:
*
* - a shared LWLock is taken on the target buffer pool partition mapping, and
@@ -92,8 +92,8 @@ check_one_block(Relation relation, ForkNumber forknum, BlockNumber blkno,
*chk_expected = *chk_found = NoComputedChecksum;
/*
- * To avoid too much overhead, the buffer will be first read without- * the locks that would guarantee the lack of false positive, as such+ * To avoid excessive overhead, the buffer will be first read without+ * the locks that would prevent false positives, as such
* events should be quite rare.
*/
Retry:
@@ -120,10 +120,10 @@ Retry:
}
/*
- * Perform a checksum check on the passed page. Returns whether the page is+ * Perform a checksum check on the passed page. Return True iff the page is
* valid or not, and assign the expected and found checksum in chk_expected and
* chk_found, respectively. Note that a page can look like new but could be
- * the result of a corruption. We still check for this case, but we can't+ * the result of corruption. We still check for this case, but we can't
* compute its checksum as pg_checksum_page() is explicitly checking for
* non-new pages, so NoComputedChecksum will be set in chk_found.
*/
@@ -139,7 +139,7 @@ check_buffer(char *buffer, uint32 blkno, uint16 *chk_expected,
if (PageIsNew(page))
{
/*
- * Check if the page is really new or if there's a corruption that+ * Check if the page is really new or if there's corruption that
* affected PageIsNew detection. Note that PageIsVerified won't try to
* detect checksum corruption in this case, so there's no risk of
* duplicated corruption report.
@@ -151,7 +151,7 @@ check_buffer(char *buffer, uint32 blkno, uint16 *chk_expected,
}
/*
- * There's a corruption, but since this affect PageIsNew, we+ * There's corruption, but since this affects PageIsNew, we
* can't compute a checksum, so set NoComputedChecksum for the
* expected checksum.
*/
@@ -218,8 +218,8 @@ check_delay_point(void)
* held. Reading with this lock is to avoid the unlikely but possible case
* that a buffer wasn't present in shared buffers when we checked but it then
* alloc'ed in shared_buffers, modified and flushed concurrently when we
- * later try to read it, leading to false positive due to torn page. Caller- * can read first the buffer without holding the target buffer mapping+ * later try to read it, leading to false positives due to a torn page. Caller+ * can first read the buffer without holding the target buffer mapping
* partition LWLock to have an optimistic approach, and reread the buffer
* from disk in case of error.
*
@@ -280,7 +280,7 @@ check_get_buffer(Relation relation, ForkNumber forknum,
checkit = false;
/*
- * Read the buffer from disk, taking on IO lock to prevent torn-page+ * Read the buffer from disk, taking an IO lock to prevent torn-page
* reads, in the unlikely event that it was concurrently dirtied and
* flushed.
*/
@@ -320,7 +320,7 @@ check_get_buffer(Relation relation, ForkNumber forknum,
/*
* Didn't find it in the buffer pool and didn't read it while holding the
* buffer mapping partition lock. We'll have to try to read it from
- * disk, after releasing the target partition lock to avoid too much+ * disk, after releasing the target partition lock to avoid excessive
* overhead. It means that it's possible to get a torn page later, so
* we'll have to retry with a suitable lock in case of error to avoid
* false positive.
diff --git a/src/backend/utils/adt/checksumfuncs.c b/src/backend/utils/adt/checksumfuncs.cindex d005b8d01f..fa5823677a 100644--- a/src/backend/utils/adt/checksumfuncs.c+++ b/src/backend/utils/adt/checksumfuncs.c@@ -1,7 +1,7 @@
/*-------------------------------------------------------------------------
*
* checksumfuncs.c
- * Functions for checksums related feature such as online verification+ * Functions for checksum related feature such as online verification
*
* Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
@@ -181,7 +181,7 @@ check_relation_fork(TupleDesc tupdesc, Tuplestorestate *tupstore,
if (check_one_block(relation, forknum, blkno, &chk_expected,
&chk_found))
{
- /* Buffer not corrupted or no worth checking, continue */+ /* Buffer not corrupted or not worth checking, continue */
continue;
}
@@ -192,7 +192,7 @@ check_relation_fork(TupleDesc tupdesc, Tuplestorestate *tupstore,
values[i++] = Int32GetDatum(forknum);
values[i++] = UInt32GetDatum(blkno);
/*
- * This can happen if a corruption makes the block appears as+ * This can happen if corruption makes the block appears as
* PageIsNew() but isn't a new page.
*/
if (chk_expected == NoComputedChecksum)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.cindex 5a51dccca9..57401580c3 100644--- a/src/backend/utils/misc/guc.c+++ b/src/backend/utils/misc/guc.c@@ -2383,7 +2383,7 @@ static struct config_int ConfigureNamesInt[] =
{
{"checksum_cost_page", PGC_USERSET, RESOURCES_CHECKSUM_DELAY,
- gettext_noop("Checksum cost for verifying a page found."),+ gettext_noop("Checksum cost for verifying a page."),
NULL
},
&ChecksumCostPage,
diff --git a/src/test/check_relation/t/01_checksums_check.pl b/src/test/check_relation/t/01_checksums_check.plindex 1ad34adcb9..2a3f2880ea 100644--- a/src/test/check_relation/t/01_checksums_check.pl+++ b/src/test/check_relation/t/01_checksums_check.pl@@ -218,7 +218,7 @@ $ENV{PGOPTIONS} = '--client-min-messages=WARNING';
my ($cmdret, $stdout, $stderr) = $node->psql('postgres', "SELECT"
. " current_setting('data_checksums')");
-is($stdout, 'on', 'Data checksums shoud be enabled');+is($stdout, 'on', 'Data checksums should be enabled');
($cmdret, $stdout, $stderr) = $node->psql('postgres', "SELECT"
. " current_setting('block_size')");
@@ -254,7 +254,7 @@ is(check_checksums_call($node, 'u_t1_id_idx'), '1', 'Can check an unlogged index
. " current_setting('data_directory') || '/' || pg_relation_filepath('t1')"
);
-isnt($stdout, '', 'A relfinode should be returned');+isnt($stdout, '', 'A relfilenode should be returned');
my $filename = $stdout;
--
2.17.0
Patches and attachments:download all (2 files, .tar.gz)0001-fixes-for-online-checksum-verification.patch (text/x-diff, 9.9K) download
From 7b9e61d640b5137dbcd0221dae0c293b705989ba Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sun, 12 Jul 2020 12:25:34 -0500
Subject: [PATCH] fixes for online checksum verification
---
doc/src/sgml/func.sgml | 20 ++++++++---------
src/backend/storage/page/checksum.c | 22 +++++++++----------
src/backend/utils/adt/checksumfuncs.c | 6 ++---
src/backend/utils/misc/guc.c | 2 +-
.../check_relation/t/01_checksums_check.pl | 4 ++--
5 files changed, 27 insertions(+), 27 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgmlindex 88efb38556..39596db193 100644--- a/doc/src/sgml/func.sgml+++ b/doc/src/sgml/func.sgml@@ -26162,7 +26162,7 @@ SELECT convert_from(pg_read_binary_file('file_in_utf8.txt'), 'UTF8');
<para>
The functions shown in <xref linkend="functions-data-sanity-table"/>
- provide means to check for health of data file in a cluster.+ provide a means to check for health of a data file in a cluster.
</para>
<table id="functions-data-sanity-table">
@@ -26179,8 +26179,8 @@ SELECT convert_from(pg_read_binary_file('file_in_utf8.txt'), 'UTF8');
<literal><function>pg_check_relation(<parameter>relation</parameter> <type>regclass</type> [, <parameter>fork</parameter> <type>text</type>])</function></literal>
</entry>
<entry><type>setof record</type></entry>
- <entry>Validate the checksums for all blocks of all or the given fork of- a given relation.</entry>+ <entry>Validate the checksum for all blocks of a relation.+ </entry>
</row>
</tbody>
</tgroup>
@@ -26190,15 +26190,15 @@ SELECT convert_from(pg_read_binary_file('file_in_utf8.txt'), 'UTF8');
<primary>pg_check_relation</primary>
</indexterm>
<para id="functions-check-relation-note" xreflabel="pg_check_relation">
- <function>pg_check_relation</function> iterates over all the blocks of a- given relation and verify their checksum. If provided,- <replaceable>fork</replaceable> should be <literal>'main'</literal> for the+ <function>pg_check_relation</function> iterates over all blocks of a+ given relation and verifies their checksums. If passed,+ <replaceable>fork</replaceable> specifies that only checksums of the given+ fork are to be verified. Fork should be <literal>'main'</literal> for the
main data fork, <literal>'fsm'</literal> for the free space map,
<literal>'vm'</literal> for the visibility map, or
- <literal>'init'</literal> for the initialization fork, and only this- specific fork will be verifies, otherwise all forks will. The function- returns the list of blocks for which the found checksum doesn't match the- expected one. See <xref+ <literal>'init'</literal> for the initialization fork.+ The function returns a list of blocks for which the computed and stored+ checksums don't match. See <xref
linkend="runtime-config-resource-checksum-verification-cost"/> for
information on how to configure cost-based verification delay. You must be
a member of the <literal>pg_read_all_stats</literal> role to use this
diff --git a/src/backend/storage/page/checksum.c b/src/backend/storage/page/checksum.cindex eb2c919c34..17cd95ec95 100644--- a/src/backend/storage/page/checksum.c+++ b/src/backend/storage/page/checksum.c@@ -36,7 +36,7 @@
* actual storage, you have to discard the operating system cache before
* running those functions.
*
- * To avoid torn page and possible false positive when reading data, and+ * To avoid torn pages and possible false positives when reading data, and to
* keeping overhead as low as possible, the following heuristics are used:
*
* - a shared LWLock is taken on the target buffer pool partition mapping, and
@@ -92,8 +92,8 @@ check_one_block(Relation relation, ForkNumber forknum, BlockNumber blkno,
*chk_expected = *chk_found = NoComputedChecksum;
/*
- * To avoid too much overhead, the buffer will be first read without- * the locks that would guarantee the lack of false positive, as such+ * To avoid excessive overhead, the buffer will be first read without+ * the locks that would prevent false positives, as such
* events should be quite rare.
*/
Retry:
@@ -120,10 +120,10 @@ Retry:
}
/*
- * Perform a checksum check on the passed page. Returns whether the page is+ * Perform a checksum check on the passed page. Return True iff the page is
* valid or not, and assign the expected and found checksum in chk_expected and
* chk_found, respectively. Note that a page can look like new but could be
- * the result of a corruption. We still check for this case, but we can't+ * the result of corruption. We still check for this case, but we can't
* compute its checksum as pg_checksum_page() is explicitly checking for
* non-new pages, so NoComputedChecksum will be set in chk_found.
*/
@@ -139,7 +139,7 @@ check_buffer(char *buffer, uint32 blkno, uint16 *chk_expected,
if (PageIsNew(page))
{
/*
- * Check if the page is really new or if there's a corruption that+ * Check if the page is really new or if there's corruption that
* affected PageIsNew detection. Note that PageIsVerified won't try to
* detect checksum corruption in this case, so there's no risk of
* duplicated corruption report.
@@ -151,7 +151,7 @@ check_buffer(char *buffer, uint32 blkno, uint16 *chk_expected,
}
/*
- * There's a corruption, but since this affect PageIsNew, we+ * There's corruption, but since this affects PageIsNew, we
* can't compute a checksum, so set NoComputedChecksum for the
* expected checksum.
*/
@@ -218,8 +218,8 @@ check_delay_point(void)
* held. Reading with this lock is to avoid the unlikely but possible case
* that a buffer wasn't present in shared buffers when we checked but it then
* alloc'ed in shared_buffers, modified and flushed concurrently when we
- * later try to read it, leading to false positive due to torn page. Caller- * can read first the buffer without holding the target buffer mapping+ * later try to read it, leading to false positives due to a torn page. Caller+ * can first read the buffer without holding the target buffer mapping
* partition LWLock to have an optimistic approach, and reread the buffer
* from disk in case of error.
*
@@ -280,7 +280,7 @@ check_get_buffer(Relation relation, ForkNumber forknum,
checkit = false;
/*
- * Read the buffer from disk, taking on IO lock to prevent torn-page+ * Read the buffer from disk, taking an IO lock to prevent torn-page
* reads, in the unlikely event that it was concurrently dirtied and
* flushed.
*/
@@ -320,7 +320,7 @@ check_get_buffer(Relation relation, ForkNumber forknum,
/*
* Didn't find it in the buffer pool and didn't read it while holding the
* buffer mapping partition lock. We'll have to try to read it from
- * disk, after releasing the target partition lock to avoid too much+ * disk, after releasing the target partition lock to avoid excessive
* overhead. It means that it's possible to get a torn page later, so
* we'll have to retry with a suitable lock in case of error to avoid
* false positive.
diff --git a/src/backend/utils/adt/checksumfuncs.c b/src/backend/utils/adt/checksumfuncs.cindex d005b8d01f..fa5823677a 100644--- a/src/backend/utils/adt/checksumfuncs.c+++ b/src/backend/utils/adt/checksumfuncs.c@@ -1,7 +1,7 @@
/*-------------------------------------------------------------------------
*
* checksumfuncs.c
- * Functions for checksums related feature such as online verification+ * Functions for checksum related feature such as online verification
*
* Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
@@ -181,7 +181,7 @@ check_relation_fork(TupleDesc tupdesc, Tuplestorestate *tupstore,
if (check_one_block(relation, forknum, blkno, &chk_expected,
&chk_found))
{
- /* Buffer not corrupted or no worth checking, continue */+ /* Buffer not corrupted or not worth checking, continue */
continue;
}
@@ -192,7 +192,7 @@ check_relation_fork(TupleDesc tupdesc, Tuplestorestate *tupstore,
values[i++] = Int32GetDatum(forknum);
values[i++] = UInt32GetDatum(blkno);
/*
- * This can happen if a corruption makes the block appears as+ * This can happen if corruption makes the block appears as
* PageIsNew() but isn't a new page.
*/
if (chk_expected == NoComputedChecksum)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.cindex 5a51dccca9..57401580c3 100644--- a/src/backend/utils/misc/guc.c+++ b/src/backend/utils/misc/guc.c@@ -2383,7 +2383,7 @@ static struct config_int ConfigureNamesInt[] =
{
{"checksum_cost_page", PGC_USERSET, RESOURCES_CHECKSUM_DELAY,
- gettext_noop("Checksum cost for verifying a page found."),+ gettext_noop("Checksum cost for verifying a page."),
NULL
},
&ChecksumCostPage,
diff --git a/src/test/check_relation/t/01_checksums_check.pl b/src/test/check_relation/t/01_checksums_check.plindex 1ad34adcb9..2a3f2880ea 100644--- a/src/test/check_relation/t/01_checksums_check.pl+++ b/src/test/check_relation/t/01_checksums_check.pl@@ -218,7 +218,7 @@ $ENV{PGOPTIONS} = '--client-min-messages=WARNING';
my ($cmdret, $stdout, $stderr) = $node->psql('postgres', "SELECT"
. " current_setting('data_checksums')");
-is($stdout, 'on', 'Data checksums shoud be enabled');+is($stdout, 'on', 'Data checksums should be enabled');
($cmdret, $stdout, $stderr) = $node->psql('postgres', "SELECT"
. " current_setting('block_size')");
@@ -254,7 +254,7 @@ is(check_checksums_call($node, 'u_t1_id_idx'), '1', 'Can check an unlogged index
. " current_setting('data_directory') || '/' || pg_relation_filepath('t1')"
);
-isnt($stdout, '', 'A relfinode should be returned');+isnt($stdout, '', 'A relfilenode should be returned');
my $filename = $stdout;
--
2.17.0
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: pryzby@telsasoft.com, rjuju123@gmail.com, masahiko.sawada@2ndquadrant.com, michael@paquier.xyz, robertmhaas@gmail.com, sawada.mshk@gmail.com
Subject: Re: Online checksums verification in the backend
In-Reply-To: <20200712173403.GI26220@telsasoft.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox