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

From: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
To: Daniel Gustafsson <daniel(at)yesql(dot)se>
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 04:52:04
Message-ID: 20180123045204.GD10053@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On Mon, Jan 22, 2018 at 12:48:45PM +0100, Daniel Gustafsson wrote:
> 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 and isCachable.

That makes sense.

> 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().

There are as well two things I have spotted on the way:
1) fillRelOptions() can safely use strcmp.
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.
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.
--
Michael

Attachment Content-Type Size
defname_strcmp-v6-fix-michael.patch text/x-diff 9.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Jeff Davis 2018-01-23 05:08:06 Re: Rangejoin rebased
Previous Message Stephen Frost 2018-01-23 04:46:34 Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump