Re: pgsql: Add TAP tests for pg_verify_checksums

Michael Paquier <michael@paquier.xyz>

From: Michael Paquier <michael@paquier.xyz>
To: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
Cc: Michael Banck <michael.banck@credativ.de>, Andres Freund <andres@anarazel.de>, Postgres hackers <pgsql-hackers@postgresql.org>, Daniel Gustafsson <daniel@yesql.se>, Magnus Hagander <magnus@hagander.net>
Date: 2018-10-14T22:41:38Z
Lists: pgsql-hackers

Attachments

On Sun, Oct 14, 2018 at 10:24:55AM -0400, Andrew Dunstan wrote:
> [snip]

Thanks a lot for the review, Andrew!

> This code now seems redundant:
> 
>          if (strcmp(fn, ".") == 0 ||
>              strcmp(fn, "..") == 0)
>              return true;

Indeed.  I have renamed skipfile() to isRelFileName on the way, which
looks better in the context.

> I would probably reverse the order of these two tests. It might not make any
> difference, assuming fn is never an empty string, but it seems more logical
> to me.
> 
>    +    /* good to go if only digits */
>    +    if (fn[pos] == '\0')
>    +        return false;
>    +    /* leave if no digits */
>    +    if (pos == 0)
>    +        return true;

No objections to that.  Done.

> It also looks to me like the check for a segment number doesn't ensure
> there is at least one digit, so "123." might pass, but I could be
> wrong. In any case, there isn't a test for that, and there probably
> should be.

You are right here.  This name pattern has been failing.  Fixed in the
attached.  I have added "123_" and "123_." as extra patterns to check.

What do you think about the updated version attached?
--
Michael

Commits

  1. Fix issues with TAP tests of pg_verify_checksums

  2. Use whitelist to choose files scanned with pg_verify_checksums

  3. Add TAP tests for pg_verify_checksums

  4. Work around Msys weakness in Testlib.pm's command_like()