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
