Re: [HACKERS] Refactoring identifier checks to consistently use strcmp

From: Daniel Gustafsson <daniel(at)yesql(dot)se>
To: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Stephen Frost <sfrost(at)snowman(dot)net>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [HACKERS] Refactoring identifier checks to consistently use strcmp
Date: 2018-01-22 11:48:45
Message-ID: 4504912B-1EE0-4148-B594-CE0AC1896445@yesql.se
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

> On 15 Jan 2018, at 02:33, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
> On Fri, Jan 12, 2018 at 11:35:48PM +0100, Daniel Gustafsson wrote:
>> On 11 Jan 2018, at 09:01, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:

>>>> One open question from this excercise is how to write a good test for
>>>> this. It can either be made part of the already existing test queries
>>>> or a separate suite. I’m leaning on the latter simply because the
>>>> case-flipping existing tests seems like something that can be cleaned
>>>> up years from now accidentally because it looks odd.
>>>
>>> Adding them into src/test/regress/ sounds like a good plan to me.
>>
>> If there is interest in this patch now that the list exists and aids review, I
>> can turn the list into a proper test that makes a little more sense than the
>> current list which is rather aimed at helping reviewers.
>
> Sorry if my words were hard to catch here. What I mean here is to add in
> each command's test file the set of commands which check the
> compatibility. There is no need to test all the options in my opinion,
> as just testing one option is enoughto show the purpose. So for example
> to cover the grounds of DefineAggregate(), you could add a set of
> commands in create_aggregate.sql. For DefineCollation(), those can go in
> collate.sql, etc.

Gotcha. I’ve added some tests to the patch. The test for CREATE FUNCTION was
omitted for now awaiting the outcome of the discussion around isStrict
isCachable.

Not sure how much they’re worth in "make check” though as it’s quite easy to
add a new option checked with pg_strcasecmp which then isn’t tested. Still, it
might aid review so definitely worth it.

cheers ./daniel

Attachment Content-Type Size
defname_strcmp-v6.patch application/octet-stream 47.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Kapila 2018-01-22 11:52:32 Re: [HACKERS] Parallel tuplesort (for parallel B-Tree index creation)
Previous Message Thomas Munro 2018-01-22 11:47:17 Re: pgsql: Add parallel-aware hash joins.