|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|
|Views:||Raw Message | Whole Thread | Download mbox|
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:
- 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
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.
|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|