Re: pgsql: Add TAP tests for pg_verify_checksums

On 10/14/2018 06:41 PM, Michael Paquier wrote:
> 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?


Looks good to me.

cheers

andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services