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-23 15:22:22
Message-ID: A6296E08-B2E1-40CD-8E4C-DC5C1305CBC1@yesql.se
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On 23 Jan 2018, at 05:52, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
>
> On Mon, Jan 22, 2018 at 12:48:45PM +0100, Daniel Gustafsson wrote:
>
>> 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.
>
> Thanks for making those, this eases the review lookups. There are a
> couple of code paths that remained untested:
> - contrib/unaccent/
> - contrib/dict_xsyn/
> - contrib/dict_int/
> - The combinations of toast reloptions is pretty particular as option
> namespaces also go through pg_strcasecmp, so I think that those should
> be tested as well (your patch includes a test for fillfactor, which is a
> good base by the way).
> - check_option for CREATE VIEW and ALTER VIEW SET.
> - ALTER TEXT SEARCH CONFIGURATION for copy/parser options.
> - CREATE TYPE AS RANGE with DefineRange().

Thanks, those are good points.

> There are as well two things I have spotted on the way:
> 1) fillRelOptions() can safely use strcmp.

Yes, I believe you’re right.

> 2) indexam.sgml mentions using pg_strcasecmp() for consistency with the
> core code when defining amproperty for an index AM. Well, with this
> patch I think that for consistency with the core code that would involve
> using strcmp instead, extension developers can of course still use
> pg_strcasecmp.

That part I’m less sure about, the propname will not be casefolded by the
parser so pg_strcasecmp() should still be the recommendation here no?

> Those are changed as well in the attached, which applies on top of your
> v6. I have added as well in it the tests I spotted were missing. If this
> looks right to you, I am fine to switch this patch as ready for
> committer, without considering the issues with isCachable and isStrict
> in CREATE FUNCTION of course.

Apart from the amproperty hunk, I’m definately +1 on adding your patch on top
of my v6 one. Thanks for all your help and review!

cheers ./daniel

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Stephen Frost 2018-01-23 15:37:11 Re: using index or check in ALTER TABLE SET NOT NULL
Previous Message Nikhil Sontakke 2018-01-23 15:01:21 Re: Logical Decoding and HeapTupleSatisfiesVacuum assumptions