Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jufsJ-0007wP-T4 for pgsql-hackers@arkaria.postgresql.org; Sun, 12 Jul 2020 17:34:52 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1jufrl-0000mP-Ju for pgsql-hackers@arkaria.postgresql.org; Sun, 12 Jul 2020 17:34:17 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jufrk-0000mI-Ls for pgsql-hackers@lists.postgresql.org; Sun, 12 Jul 2020 17:34:17 +0000 Received: from mail-io1-xd42.google.com ([2607:f8b0:4864:20::d42]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1jufrc-0001RM-3m for pgsql-hackers@lists.postgresql.org; Sun, 12 Jul 2020 17:34:14 +0000 Received: by mail-io1-xd42.google.com with SMTP id q74so11115943iod.1 for ; Sun, 12 Jul 2020 10:34:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=telsasoft-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=C7Xk1AjwXGliOD2d8KE2K4PTgNbLXQg2vkdzLh63nkU=; b=d8YzfVXn9VRDpDkTlYRmWZX6gZnEwfWVytdD2+zSzPxUknQ6D3biaNsJ6F+TlcOFzL fcqZw+ubhj29jdpi/Uw1j3PhdQwkwWCeliwAT6MP9j95pdjP6WJOUy0TpE4l217Wwato vH4lz9zKzGEIuNTfZTVqf1gJMARr6YByxYEYqXTUdZLvdq0Mh6C1oelBWzUgYkvRJU/c gAJ+x/E6PmSlpSmkBkatWVQh8CqlSg06y9kshLOOwJkkUrNnY8XYPAV9Yl8+jJbe8lSI zfuUZ5/6EMnG2SpJo3tJtZBuJmLwh0HlcqhcD6bPDs1pSp+tEko3nWfO2RJVBXwlaExU wBSA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=C7Xk1AjwXGliOD2d8KE2K4PTgNbLXQg2vkdzLh63nkU=; b=ff7qIp84XNaDEYSO8AIzAH/kghOxye0l8/YEYM+5BoVIVjc1/CPhyYwdUNI/hugm0I 83EDfccJWXaRWQnunO/PBP4IUj3phP5gYZHXyLg28ezL33XOuJfVxAi/cd9352SzK/PR BwUt+uSJ73LU7JJg64bE7y4dYO02XZRxZVeT3QC0+qLC0weatv81Q+207UM+jlLgtDyz hgICrvqjSRnsfbw9f/dN5T9AEJe8rcfTzpPsjpK0DbyGu35FiRcHTBzGTaXPKwYF/2Ch fs3O3dvwKN+CR85SKqalnwe0QNT/V0xnhwbo35lAY6Qm/tZXWsK7+exhacipbh5vgvdm jpOw== X-Gm-Message-State: AOAM530HYdtRLGQFINUfrYdVWXufORtqvnvk5QcS4kVXZc2rq1gVcgyj R90CLcL9SPltvVo7AT6Tva7K0w== X-Google-Smtp-Source: ABdhPJz4XBOlm7bWAaXNr+kBieMq6AuxAyc0ljf6L0qxmDM6x1eUt+YNQBXuOGlSyyaNXWQ7pLp45g== X-Received: by 2002:a05:6638:62e:: with SMTP id h14mr78549332jar.82.1594575246518; Sun, 12 Jul 2020 10:34:06 -0700 (PDT) Received: from pryzbyj.telsasoft (charmander.telsasoft.com. [50.244.222.1]) by smtp.gmail.com with ESMTPSA id c14sm2362360ild.41.2020.07.12.10.34.04 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Sun, 12 Jul 2020 10:34:05 -0700 (PDT) Received: by pryzbyj.telsasoft (Postfix, from userid 1000) id 4E567800144; Sun, 12 Jul 2020 12:34:03 -0500 (CDT) Date: Sun, 12 Jul 2020 12:34:03 -0500 From: Justin Pryzby To: Julien Rouhaud Cc: Masahiko Sawada , Michael Paquier , Robert Haas , pgsql-hackers@lists.postgresql.org, Masahiko Sawada Subject: Re: Online checksums verification in the backend Message-ID: <20200712173403.GI26220@telsasoft.com> References: <20200318101055.GA36918@nol> <20200328121858.GA13605@nol> <20200403093911.GB1206@nol> <20200404090428.GD1206@nol> <20200405084355.GF1206@nol> <20200405094459.GG1206@nol> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="xXmbgvnjoT4axfJE" Content-Disposition: inline In-Reply-To: <20200405094459.GG1206@nol> User-Agent: Mutt/1.9.4 (2018-02-28) List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --xXmbgvnjoT4axfJE Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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'); The functions shown in - 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. @@ -26179,8 +26179,8 @@ SELECT convert_from(pg_read_binary_file('file_in_utf8.txt'), 'UTF8'); pg_check_relation(relation regclass [, fork text])setof record - Validate the checksums for all blocks of all or the given fork of - a given relation. + Validate the checksum for all blocks of a relation. + @@ -26190,15 +26190,15 @@ SELECT convert_from(pg_read_binary_file('file_in_utf8.txt'), 'UTF8'); pg_check_relation - pg_check_relation iterates over all the blocks of a - given relation and verify their checksum. If provided, - fork should be 'main' for the + pg_check_relation iterates over all blocks of a + given relation and verifies their checksums. If passed, + fork specifies that only checksums of the given + fork are to be verified. Fork should be 'main' for the main data fork, 'fsm' for the free space map, 'vm' for the visibility map, or - 'init' 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 'init' for the initialization fork. + The function returns a list of blocks for which the computed and stored + checksums don't match. See for information on how to configure cost-based verification delay. You must be a member of the pg_read_all_stats 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 --xXmbgvnjoT4axfJE Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0001-fixes-for-online-checksum-verification.patch" From 7b9e61d640b5137dbcd0221dae0c293b705989ba Mon Sep 17 00:00:00 2001 From: Justin Pryzby 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.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'); The functions shown in - 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.
@@ -26179,8 +26179,8 @@ SELECT convert_from(pg_read_binary_file('file_in_utf8.txt'), 'UTF8'); pg_check_relation(relation regclass [, fork text])setof record - Validate the checksums for all blocks of all or the given fork of - a given relation. + Validate the checksum for all blocks of a relation. + @@ -26190,15 +26190,15 @@ SELECT convert_from(pg_read_binary_file('file_in_utf8.txt'), 'UTF8'); pg_check_relation - pg_check_relation iterates over all the blocks of a - given relation and verify their checksum. If provided, - fork should be 'main' for the + pg_check_relation iterates over all blocks of a + given relation and verifies their checksums. If passed, + fork specifies that only checksums of the given + fork are to be verified. Fork should be 'main' for the main data fork, 'fsm' for the free space map, 'vm' for the visibility map, or - 'init' 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 'init' for the initialization fork. + The function returns a list of blocks for which the computed and stored + checksums don't match. See for information on how to configure cost-based verification delay. You must be a member of the pg_read_all_stats 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 --xXmbgvnjoT4axfJE--