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
signature.asc
Description: PGP signature