Re: Online checksums verification in the backend

From: Justin Pryzby <pryzby(at)telsasoft(dot)com>
To: Julien Rouhaud <rjuju123(at)gmail(dot)com>
Cc: Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Robert Haas <robertmhaas(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
Subject: Re: Online checksums verification in the backend
Date: 2020-07-12 17:34:03
Message-ID: 20200712173403.GI26220@telsasoft.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Small language fixes in comments and user-facing docs.

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 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.c
index 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.c
index 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.c
index 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.pl
index 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

Attachment Content-Type Size
0001-fixes-for-online-checksum-verification.patch text/x-diff 9.9 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andrey Lepikhov 2020-07-12 17:46:16 Re: [POC] Fast COPY FROM command for the table with foreign partitions
Previous Message Dilip Kumar 2020-07-12 16:26:34 Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions