Re: pgsql: Add TAP tests for pg_verify_checksums

Greetings,

On Sat, Oct 20, 2018 at 00:31 Michael Paquier <michael(at)paquier(dot)xyz> wrote:

> On Fri, Oct 19, 2018 at 08:50:04PM -0700, Andres Freund wrote:
> > On 2018-10-20 12:39:55 +0900, Michael Paquier wrote:
> >> So what I think we ought to do is the following:
> >> - Start a new thread, this one about TAP tests is not adapted.
> >> - Add in src/common/relpath.c the API from d55241af called
> >> isRelFileName(), make use of it in the base backup code, and basically
> >> remove is_checksummed_file() and the checksum skip list.
> >
> > I think it probably shouldn't quite be that as an API. The code should
> > not just check whether the file matches a pattern, but also importantly
> > needs to exclude files that are in a temp tablespace. isRelFileName()
> > doesn't quite describe an API meaning that. I assume we should keep
> > something like isRelFileName() but use an API ontop of that that also
> > exclude temp files / relations.
>
> From what I can see we would need to check for a couple of patterns if
> we go to this extent:
> - Look for PG_TEMP_FILE_PREFIX and exclude those.
> - looks_like_temp_rel_name() which checks for temp files names. This is
> similar to isRelFileName except that it works on temporary files.
> Moving it to relpath.c and renaming it IsTempRelFileName is an idea.
> But this one would not be necessary as isRelFileName discards temporary
> relations, no?
> - parse_filename_for_nontemp_relation() is also too similar to
> isRelFileName().
>
> At the end, do we really need to do anything more than adding some
> checks on PG_TEMP_FILE_PREFIX? I am not sure that there is much need
> for a global API like isChecksummedFile for only those two places. I
> have already a patch doing the work of moving isRelFileName() into
> src/common/. Adding one check on PG_TEMP_FILE_PREFIX is not much work
> on top of it.

I’m not at my computer at the moment so may not be entirely following the
question here but to be clear, whatever we do here will have downstream
impact into other tools, such as pgbackrest, and therefore we definitely
want to have the code in libpgcommon so that these external tools can
leverage it and know that they’re doing what PG does.

I’d also like to give David Steele a chance to comment on the specific API,
and any other backup tools authors, which I don’t think we should be
rushing into anyway and I would think we’d only put into master..

Thanks!

Stephen