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

From: Daniel Gustafsson <daniel(at)yesql(dot)se>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: 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-06 14:41:20
Message-ID: C4CC38F4-3D8D-4CE0-BA31-5B2DD1504C57@yesql.se
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On 6 Feb 2019, at 12:37, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Wed, Feb 06, 2019 at 10:50:27AM +0100, Daniel Gustafsson wrote:
>> I still think we should enforce one-or-more matching on the OWNER part as well,
>> since matching zero would be a syntax error. There are more .* matches but
>> I’ve only touched the ones which match SQL, since there is a defined grammar to
>> rely on there. The attached patch does that on top of your commit.
>
> - regexp => qr/^COMMENT ON DATABASE postgres IS .*;/m,
> + regexp => qr/^COMMENT ON DATABASE postgres IS .+;/m,
> So... With what's on HEAD the current regex means that all these are
> correct commands:
> COMMENT ON DATABASE postgres is ;
> COMMENT ON DATABASE postgres is foo;
> COMMENT ON DATABASE postgres is foo;
> And for the first one that's obviously wrong.
>
> So what you are suggesting is to actually make the first pattern
> something to complain about, right? And ".+" this makes sure that at
> least one character is present, while for ".*" it is fine to have zero
> characters. What you are suggesting looks right if I understood that
> right.

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.

cheers ./daniel

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2019-02-06 15:09:22 Re: bug tracking system
Previous Message Andres Freund 2019-02-06 14:29:15 Re: Synchronize with imath upstream