Re: Online checksums verification in the backend

On Tue, Jul 14, 2020 at 11:08:08AM +0200, Julien Rouhaud wrote:
> On Sun, Jul 12, 2020 at 12:34:03PM -0500, Justin Pryzby wrote:
>> Small language fixes in comments and user-facing docs.
>
> Thanks a lot! I just fixed a small issue (see below), PFA updated v10.

Sawada-san, you are registered as a reviewer of this patch. Are you
planning to look at it? If you are busy lately, that's fine as well
(congrats!). In this case it could be better to unregister from the
CF app for this entry.

I am refreshing my mind here, but here are some high-level comments
for now...

+#include "postgres.h"
+
+#include "access/tupdesc.h"
+#include "common/relpath.h"
#include "storage/block.h"
+#include "utils/relcache.h"
+#include "utils/tuplestore.h"
[...]
+extern bool check_one_block(Relation relation, ForkNumber forknum,
+ BlockNumber blkno, uint16 *chk_expected,
+ uint16 *chk_found);
I don't think that it is a good idea to add this much to checksum.h
as these are includes coming mainly from the backend. Note that
pg_checksum_page() is a function designed to be also available for
frontend tools, with checksum.h something that can be included in
frontends. This would mean that we could move all the buffer lookup
APIs directly to checksumfuncs.c, or move that into a separate file
closer to the location.

+ * A zero checksum can never be computed, see pg_checksum_page() */
+#define NoComputedChecksum 0
Wouldn't it be better to rename that something like
InvalidPageChecksum, and make use of it in pg_checksum_page()? It
would be more consistent with the naming of transaction IDs, OIDs or
even XLogRecPtr. And that could be a separate patch.

+++ b/src/test/check_relation/t/01_checksums_check.pl
@@ -0,0 +1,276 @@
+use strict;
+use warnings;
It could be better to move that to src/test/modules/, so as it could
be picked more easily by MSVC scripts in the future. Note that if we
apply the normal naming convention here this should be named
001_checksum_check.pl.

+subdir = src/test/check_relation
+top_builddir = ../../..
+include $(top_builddir)/src/Makefile.global
Let's use a Makefile shaped in a way similar to modules/test_misc that
makes use of TAP_TESTS = 1. There is the infra, let's rely on it for
the regression tests.

+ pg_usleep(msec * 1000L);
Could it be possible to add a wait event here? It would be nice to be
able to monitor that in pg_stat_activity.

+if (exists $ENV{MY_PG_REGRESS})
+{
+ $ENV{PG_REGRESS} = $ENV{MY_PG_REGRESS};
+}
What is MY_PG_REGRESS for? A remnant from an external makefile
perhaps?

+ /*
+ * If we get a failure and the buffer wasn't found in shared buffers,
+ * reread the buffer with suitable lock to avoid false positive. See
+ * check_get_buffer for more details.
+ */
+ if (!found_in_sb && !force_lock)
+ {
+ force_lock = true;
+ goto Retry;
+ }
As designed, we have a risk of false positives with a torn page in the
first loop when trying to look for a given buffer as we would try to
use smgrread() without a partition lock. This stresses me a bit, and
false positives could scare users easily. Could we consider first a
safer approach where we don't do that, and just read the page while
holding the partition lock? OK, that would be more expensive, but at
least that's safe in any case. My memory of this patch is a bit
fuzzy, but this is itching me and this is the heart of the problem
dealt with here :)
--
Michael