Re: More issues with pg_verify_checksums and checksum verification in base backups

Mmm. It took too long time than expected because I was repeatedly
teased by git..

At Wed, 24 Oct 2018 14:31:37 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20181024053137.GL1658@paquier.xyz>
> On Sun, Oct 21, 2018 at 08:56:32PM -0400, Stephen Frost wrote:
> > All of this pie-in-the-sky about what pluggable storage might have is
> > just hand-waving, in my opinion, and not worth much more than that.  I
> > hope (and suspect..) that the actual pluggable storage that's being
> > worked on doesn't do any of this "just drop a file somewhere" because
> > there's a lot of downsides to it- and if it did, it wouldn't be much
> > more than what we can do with an FDW, so why go through and add it?
> 
> Well, there is no point in enforcing a rule that something is forbidden
> if if was never implied and never documented (the rule here is to be
> able to drop custom files into global/, base/ or pg_tblspc.).  Postgres
> is highly-customizable, I would prefer if features in core are designed
> so as we keep things extensible, the checksum verification for base
> backup on the contrary restricts things.
> 
> So, do we have other opinions about this thread?

I believe of freedom for anyone of dropping any files into
anywhere in $PGDATA if he thinks it sefe for the time
being. Changes in core sometimes breaks some extensions and they
used to be 'fixed' in the manner or claim for a replacement to
core. This is the same for changes of (undocumented) APIs. I
think things have been going around in this manner for years and
I don't think core side is unable to define a definite border of
what extensions are allowed to do since we are not knowledgeable
of all extensions that will be created in future or that have
created.

So I'm +1 for the Michael's current patch as (I think) we can't
make visible or large changes.


That said, I agree with Stephen's concern on the point we could
omit requried files in future, but on the other hand I don't want
random files are simply rejected.

So I propose to sort files into roughly three categories. One is
files we know of but to skip. Second is files we know of and need
checksum verification. The last is the files we just don't know of.

In the attached PoC (checksum_)find_file_type categorizes a file
into 6 (or 7) categories.

ENTRY_TO_IGNORE: to be always ignored, for "." and "..".
FILE_TO_SKIP:    we know of and to skip. files in the black list.
DIR_TO_SKIP:     directories same to the above.
DIR_TO_SCAN:	 we know any file to scan may be in it.
HEAP_TO_SCAN:	 we know it has checksums in heap format.
FILE_UNKNOWN:    we don't know of.
(STAT_FAILED:    stat filed for the file)

Among these types, what are related to the discussion above are
FILE_TO_SKIP, HEAP_TO_SCAN and FILE_UNKNOWN. Others are for
controlling search loop in pg_verify_checksums.

Based on this categorization, pg_verify_checksum's result is shown as:

> Checksum scan completed
> Data checksum version: 1
> Files scanned:  1095
> Blocks scanned: 2976
> Bad checksums:  0
+ Files skipped: 8
+ Unknown files skipped: 1
+ Skipped directories: 1

(Data checksum version is bonded with heap checksums..)

If this sort of change is acceptable for us, I believe it
comforms everyone here's wishes. If skipped unknown is not zero
on a buildfarm animal, it is a sign of something is forgotten.

The second attached (also PoC) is common'ize the file sorter. The
function is moved to src/common/file_checksums.c and both
pg_verify_checksums.c and basebackup.c uses it.  With
log_min_messages=debug1, you will see the following message for
unkown files during basebackup.

> DEBUG:  checksum verification was skipped for unknown file: ./base/hoge

This changed one behavior of the tool. -r now can take
'dbid/relid' addition to just 'relid'.  I think we could have
.verify_checksum.exlucde file so that extensions can declare
files.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center