Hi Alvaro, On Wed, Sep 25, 2019 at 06:59:24PM +0000, Alvaro Herrera wrote: > Support reloptions of enum type > > All our current in core relation options of type string (not many, > admittedly) behave in reality like enums. But after seeing an > implementation for enum reloptions, it's clear that strings are messier, > so introduce the new reloption type. Switch all string options to be > enums instead. > > Fortunately we have a recently introduced test module for reloptions, so > we don't lose coverage of string reloptions, which may still be used by > third-party modules. > > Authors: Nikolay Shaplov, Álvaro Herrera > Reviewed-by: Nikita Glukhov, Aleksandr Parfenov > Discussion: https://postgr.es/m/43332102.S2V5pIjXRx@x200m
This part from commit 773df88 is incorrect:
@@ -661,6 +700,13 @@ allocate_reloption(bits32 kinds, int type, const
char *name, const char *desc,
newoption->type = type;
newoption->lockmode = lockmode;
+ /*
+ * Set the default lock mode for this option. There is no actual way
+ * for a module to enforce it when declaring a custom relation option,
+ * so just use the highest level, which is safe for all cases.
+ */
+ newoption->lockmode = AccessExclusiveLock;
Any caller of allocate_reloption() passes down its own lockmode
definition, hence you are removing the pluggability of the API. I
think that you just got trapped by an incorrect rebase.
Do you mind if I apply the attached to fix the issue? Or perhaps you
would prefer fixing the issue yourself? I noted some inconsistencies
with the rest while on it (please see attached).
--
Michael
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c index 85627a05a3..b5072c00fe 100644 --- a/src/backend/access/common/reloptions.c +++ b/src/backend/access/common/reloptions.c @@ -700,13 +700,6 @@ allocate_reloption(bits32 kinds, int type, const char *name, const char *desc, newoption->type = type; newoption->lockmode = lockmode; - /* - * Set the default lock mode for this option. There is no actual way - * for a module to enforce it when declaring a custom relation option, - * so just use the highest level, which is safe for all cases. - */ - newoption->lockmode = AccessExclusiveLock; - MemoryContextSwitchTo(oldcxt); return newoption; diff --git a/src/test/modules/dummy_index_am/README b/src/test/modules/dummy_index_am/README index 7bcdec56f3..61f3360fd1 100644 --- a/src/test/modules/dummy_index_am/README +++ b/src/test/modules/dummy_index_am/README @@ -8,4 +8,5 @@ This includes tests for all relation option types: - boolean - integer - real +- enum - strings (with and without NULL as default) diff --git a/src/test/modules/dummy_index_am/expected/reloptions.out b/src/test/modules/dummy_index_am/expected/reloptions.out index 99742f6fdc..c873a80bb7 100644 --- a/src/test/modules/dummy_index_am/expected/reloptions.out +++ b/src/test/modules/dummy_index_am/expected/reloptions.out @@ -20,6 +20,7 @@ CREATE INDEX dummy_test_idx ON dummy_test_tab option_bool = false, option_int = 5, option_real = 3.1, + option_enum = 'two', option_string_val = NULL, option_string_null = 'val'); NOTICE: new option value for string parameter null @@ -32,9 +33,10 @@ SELECT unnest(reloptions) FROM pg_class WHERE relname = 'dummy_test_idx'; option_bool=false option_int=5 option_real=3.1 + option_enum=two option_string_val=null option_string_null=val -(5 rows) +(6 rows) -- ALTER INDEX .. SET ALTER INDEX dummy_test_idx SET (option_int = 10); diff --git a/src/test/modules/dummy_index_am/sql/reloptions.sql b/src/test/modules/dummy_index_am/sql/reloptions.sql index 0172cf094e..6749d763e6 100644 --- a/src/test/modules/dummy_index_am/sql/reloptions.sql +++ b/src/test/modules/dummy_index_am/sql/reloptions.sql @@ -20,6 +20,7 @@ CREATE INDEX dummy_test_idx ON dummy_test_tab option_bool = false, option_int = 5, option_real = 3.1, + option_enum = 'two', option_string_val = NULL, option_string_null = 'val'); -- Silence again validation checks for strings until the end of the test.
signature.asc
Description: PGP signature
