Re: Tighten up a few overly lax regexes in pg_dump's tap tests

From: Daniel Gustafsson <daniel(at)yesql(dot)se>
To: Tels <nospam-pg-abuse(at)bloodgate(dot)com>
Cc: Michael Paquier <michael(at)paquier(dot)xyz>, David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Tighten up a few overly lax regexes in pg_dump's tap tests
Date: 2019-02-07 14:33:54
Message-ID: A5A8676D-6CE7-4BCD-898E-D7AA3C06AD23@yesql.se
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On 7 Feb 2019, at 13:55, Tels <nospam-pg-abuse(at)bloodgate(dot)com> wrote:
> On Wed, February 6, 2019 8:10 pm, Michael Paquier wrote:
>> On Wed, Feb 06, 2019 at 03:41:20PM +0100, Daniel Gustafsson wrote:
>>> Correct. One could argue that the regex is still suboptimal since
>>> “COMMENT ON
>>> DATABASE postgres IS ;” will be matched as well, but there I think the
>>> tradeoff
>>> for readability wins.
>>
>> Okay, that looks like an improvement anyway, so committed after going
>> over the tests for similar problems, and there was one for CREATE
>> DATABASE and DROP ROLE. It is possible to have a regex which tells as
>> at least one non-whitespace character, but from what I can see these
>> don't really improve the readability.
>
> Sorry for being late to the party, but it just occured to me that while
> ".+" is definitely an improvement over ".*", it isn't foolproof either,
> as it also matches ";”.

Correct. The idea was to maintain readability while making the regex a bit
better, without any claims to make it perfect. One can argue whether or not
that’s enough, but IMO keeping the tests as readable as possible is preferrable
when it comes to the tests in question, as they aren’t matching against user
supplied SQL but machine generated.

cheers ./daniel

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2019-02-07 14:49:05 Re: phase out ossp-uuid?
Previous Message Sergei Kornilov 2019-02-07 14:28:05 Re: REINDEX CONCURRENTLY 2.0