> On 23 Jan 2018, at 05:52, Michael Paquier <michael.paqu...@gmail.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
