Re: pgsql: Add TAP tests for pg_verify_checksums

Hi,

On 2018-10-20 13:30:46 +0900, Michael Paquier 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?

I think it's not good to have those necessary intermingled in an exposed
function.

> At the end, do we really need to do anything more than adding some
> checks on PG_TEMP_FILE_PREFIX?

I think we also need the exclusion list basebackup.c has that generally
skips files. They might be excluded anyway, but I think it'd be safer to
make sure.

> I am not sure that there is much need for a global API like
> isChecksummedFile for only those two places.

Seems likely that other tools would want to have access too.

Greetings,

Andres Freund