Re: pgsql: Add TAP tests for pg_verify_checksums

On 10/13/2018 10:00 AM, Andrew Dunstan wrote:
>
>
> On 10/13/2018 04:30 AM, Michael Paquier wrote:
>> On Fri, Oct 12, 2018 at 12:14:48PM +0200, Michael Banck wrote:
>>> On Fri, Oct 12, 2018 at 03:05:43PM +0900, Michael Paquier wrote:
>>>> On Fri, Oct 12, 2018 at 12:11:58PM +0900, Michael Paquier wrote:
>>>>> Agreed.  I am just working on a patch for v11- which uses a
>>>>> whitelist-based method instead of what is present now. Reverting the
>>>>> tests to put the buildfarm to green could be done, but that's not the
>>>>> root of the problem.
>>> I think that's the right solution; the online verification patch adds
>>> even more logic to the blacklist so getting rid of it in favor of a
>>> whitelist is +1 with me.
>> Thanks Michael for the input!
>>
>>>> So, I have coded this thing, and finish with the attached.  The
>>>> following file patterns are accepted for the checksums:
>>>> <digits>.<segment>
>>>> <digits>_<forkname>
>>>> <digits>_<forkname>.<segment>
>>>> I have added some tests on the way to make sure that all the patterns
>>>> get covered.  Please note that this skips the temporary files.
>>> Maybe also add some correct (to be scanned) but non-empty garbage files
>>> later on that it should barf on?
>> I was not sure about doing that in the main patch so I tweaked manually
>> the test to make sure that the tool still complained with "could not
>> read block" as it should.  That's easy enough to add, so I'll add them
>> with multiple file patterns.  Those are cheap checks as well if they are
>> placed in global/.
>>
>> Another problem that the patch has is that it is not using
>> forkname_to_number() to scan for all the fork types, and I forgot init
>> forks in the previous version.  Using forkname_to_number() also makes
>> the tool more bug-proof, it is also not complicated to plug into the
>> existing patch.
>>
>> Anyway, I have a bit of a problem here, which prevents me to stay in
>> front of a computer or to look at a screen for more than a couple of
>> minutes in a row for a couple of days at least, and I don't like to keep
>> the buildfarm unhappy for the time being.  There are three options:
>> 1) Revert the TAP tests of pg_verify_checksums.
>> 2) Push the patch which adds new entries for EXEC_BACKEND files in the
>> skip list.  That's a short workaround, and that would allow default
>> deployments of Postgres to use the tool.
>> 3) Finish the patch with the whitelist approach.
>>
>> I can do 1) or 2) in my condition.  3) requires more work than I can do
>> now, though the patch to do is getting in shape, so the buildfarm would
>> stay unhappy.  Any preference of the course of action to take?
>
>
>
> I have disabled the test temporarily on my two animals since I want to
> make sure they are working OK with other changes, and we know what the
> problem is. Andres might want to do that with his animal also just add
> "--skip-steps=pg_verify_checksums-check" to the command line.
>
> If you want to throw what you have for 3) over to wall to me I can see
> if I can finish it.
>

It occurred to me that a pretty simple fix could just be to blacklist
everything that didn't start with a digit. The whitelist approach is
probably preferable  ... depends how urgent we see this as.

cheers

andrew

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