On Fri, Sep 20, 2019 at 12:40:51PM +0530, Kuntal Ghosh wrote: > Okay. Sounds good.
Thanks for the review. Attached is the patch set I am planning to commit. I'll wait after the tag of this week as the first patch needs to go down to 9.6, the origin of the bug being 47167b7. The second patch would go only to HEAD, as discussed. -- Michael
From c0c2f75ae0b4fa3e6959a1157d400e316f40ada0 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Mon, 23 Sep 2019 15:20:37 +0900 Subject: [PATCH v2 1/3] Fix failure with lock mode used for custom relation options Relation options can use a custom lock mode since 47167b7, which has lowered the lock available for some autovacuum parameters, however it forgot to consider custom relation options. This causes failures with ALTER TABLE SET when changing a custom relation option, as its lock is not defined. The existing APIs to define a custom reloption does not allow to define a custom lock mode, so enforce its initialization to AccessExclusiveMode which is safe enough in all cases. An upcoming patch will extend the existing APIs to allow a custom lock mode to be defined. The problem can be reproduced with bloom indexes, so add a test there. Reported-by: Nikolay Sharplov Analyzed-by: Thomas Munro, Michael Paquier Author: Michael Paquier Reviewed-by: Kuntal Ghosh Discussion: https://postgr.es/m/20190920013831.gd1...@paquier.xyz Backpatch-through: 9.6 --- contrib/bloom/expected/bloom.out | 1 + contrib/bloom/sql/bloom.sql | 1 + src/backend/access/common/reloptions.c | 7 +++++++ 3 files changed, 9 insertions(+) diff --git a/contrib/bloom/expected/bloom.out b/contrib/bloom/expected/bloom.out index 5ab9e34f82..dae12a7d3e 100644 --- a/contrib/bloom/expected/bloom.out +++ b/contrib/bloom/expected/bloom.out @@ -5,6 +5,7 @@ CREATE TABLE tst ( ); INSERT INTO tst SELECT i%10, substr(md5(i::text), 1, 1) FROM generate_series(1,2000) i; CREATE INDEX bloomidx ON tst USING bloom (i, t) WITH (col1 = 3); +ALTER INDEX bloomidx SET (length=80); SET enable_seqscan=on; SET enable_bitmapscan=off; SET enable_indexscan=off; diff --git a/contrib/bloom/sql/bloom.sql b/contrib/bloom/sql/bloom.sql index 32755f2b1a..4733e1e705 100644 --- a/contrib/bloom/sql/bloom.sql +++ b/contrib/bloom/sql/bloom.sql @@ -7,6 +7,7 @@ CREATE TABLE tst ( INSERT INTO tst SELECT i%10, substr(md5(i::text), 1, 1) FROM generate_series(1,2000) i; CREATE INDEX bloomidx ON tst USING bloom (i, t) WITH (col1 = 3); +ALTER INDEX bloomidx SET (length=80); SET enable_seqscan=on; SET enable_bitmapscan=off; diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c index 20f4ed3c38..b59e606771 100644 --- a/src/backend/access/common/reloptions.c +++ b/src/backend/access/common/reloptions.c @@ -659,6 +659,13 @@ allocate_reloption(bits32 kinds, int type, const char *name, const char *desc) newoption->namelen = strlen(name); newoption->type = type; + /* + * 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; -- 2.23.0
From c495f03450b6c4727b3d2f44b3509916f0ee73ae Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Mon, 23 Sep 2019 15:28:02 +0900 Subject: [PATCH v2 2/3] Allow definition of lock mode for custom reloptions Relation options can define a lock mode other than AccessExclusiveMode since 47167b7, but modules defining custom relation options did not really have a way to enforce that. Correct that by extending the current API set so as modules can define a custom lock mode. Author: Michael Paquier Reviewed-by: Kuntal Ghosh Discussion: https://postgr.es/m/20190920013831.gd1...@paquier.xyz --- contrib/bloom/blutils.c | 6 ++++-- src/backend/access/common/reloptions.c | 28 +++++++++++--------------- src/include/access/reloptions.h | 11 ++++++---- 3 files changed, 23 insertions(+), 22 deletions(-) diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c index cc1670934f..dbb24cb5b2 100644 --- a/contrib/bloom/blutils.c +++ b/contrib/bloom/blutils.c @@ -60,7 +60,8 @@ _PG_init(void) /* Option for length of signature */ add_int_reloption(bl_relopt_kind, "length", "Length of signature in bits", - DEFAULT_BLOOM_LENGTH, 1, MAX_BLOOM_LENGTH); + DEFAULT_BLOOM_LENGTH, 1, MAX_BLOOM_LENGTH, + AccessExclusiveLock); bl_relopt_tab[0].optname = "length"; bl_relopt_tab[0].opttype = RELOPT_TYPE_INT; bl_relopt_tab[0].offset = offsetof(BloomOptions, bloomLength); @@ -71,7 +72,8 @@ _PG_init(void) snprintf(buf, sizeof(buf), "col%d", i + 1); add_int_reloption(bl_relopt_kind, buf, "Number of bits generated for each index column", - DEFAULT_BLOOM_BITS, 1, MAX_BLOOM_BITS); + DEFAULT_BLOOM_BITS, 1, MAX_BLOOM_BITS, + AccessExclusiveLock); bl_relopt_tab[i + 1].optname = MemoryContextStrdup(TopMemoryContext, buf); bl_relopt_tab[i + 1].opttype = RELOPT_TYPE_INT; diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c index b59e606771..3b8517efea 100644 --- a/src/backend/access/common/reloptions.c +++ b/src/backend/access/common/reloptions.c @@ -621,7 +621,8 @@ add_reloption(relopt_gen *newoption) * (for types other than string) */ static relopt_gen * -allocate_reloption(bits32 kinds, int type, const char *name, const char *desc) +allocate_reloption(bits32 kinds, int type, const char *name, const char *desc, + LOCKMODE lockmode) { MemoryContext oldcxt; size_t size; @@ -658,13 +659,7 @@ allocate_reloption(bits32 kinds, int type, const char *name, const char *desc) newoption->kinds = kinds; newoption->namelen = strlen(name); newoption->type = type; - - /* - * 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; + newoption->lockmode = lockmode; MemoryContextSwitchTo(oldcxt); @@ -676,12 +671,13 @@ allocate_reloption(bits32 kinds, int type, const char *name, const char *desc) * Add a new boolean reloption */ void -add_bool_reloption(bits32 kinds, const char *name, const char *desc, bool default_val) +add_bool_reloption(bits32 kinds, const char *name, const char *desc, + bool default_val, LOCKMODE lockmode) { relopt_bool *newoption; newoption = (relopt_bool *) allocate_reloption(kinds, RELOPT_TYPE_BOOL, - name, desc); + name, desc, lockmode); newoption->default_val = default_val; add_reloption((relopt_gen *) newoption); @@ -693,12 +689,12 @@ add_bool_reloption(bits32 kinds, const char *name, const char *desc, bool defaul */ void add_int_reloption(bits32 kinds, const char *name, const char *desc, int default_val, - int min_val, int max_val) + int min_val, int max_val, LOCKMODE lockmode) { relopt_int *newoption; newoption = (relopt_int *) allocate_reloption(kinds, RELOPT_TYPE_INT, - name, desc); + name, desc, lockmode); newoption->default_val = default_val; newoption->min = min_val; newoption->max = max_val; @@ -712,12 +708,12 @@ add_int_reloption(bits32 kinds, const char *name, const char *desc, int default_ */ void add_real_reloption(bits32 kinds, const char *name, const char *desc, double default_val, - double min_val, double max_val) + double min_val, double max_val, LOCKMODE lockmode) { relopt_real *newoption; newoption = (relopt_real *) allocate_reloption(kinds, RELOPT_TYPE_REAL, - name, desc); + name, desc, lockmode); newoption->default_val = default_val; newoption->min = min_val; newoption->max = max_val; @@ -736,7 +732,7 @@ add_real_reloption(bits32 kinds, const char *name, const char *desc, double defa */ void add_string_reloption(bits32 kinds, const char *name, const char *desc, const char *default_val, - validate_string_relopt validator) + validate_string_relopt validator, LOCKMODE lockmode) { relopt_string *newoption; @@ -745,7 +741,7 @@ add_string_reloption(bits32 kinds, const char *name, const char *desc, const cha (validator) (default_val); newoption = (relopt_string *) allocate_reloption(kinds, RELOPT_TYPE_STRING, - name, desc); + name, desc, lockmode); newoption->validate_cb = validator; if (default_val) { diff --git a/src/include/access/reloptions.h b/src/include/access/reloptions.h index 6d392e4d5a..4b82c6370a 100644 --- a/src/include/access/reloptions.h +++ b/src/include/access/reloptions.h @@ -247,13 +247,16 @@ typedef struct extern relopt_kind add_reloption_kind(void); extern void add_bool_reloption(bits32 kinds, const char *name, const char *desc, - bool default_val); + bool default_val, LOCKMODE lockmode); extern void add_int_reloption(bits32 kinds, const char *name, const char *desc, - int default_val, int min_val, int max_val); + int default_val, int min_val, int max_val, + LOCKMODE lockmode); extern void add_real_reloption(bits32 kinds, const char *name, const char *desc, - double default_val, double min_val, double max_val); + double default_val, double min_val, double max_val, + LOCKMODE lockmode); extern void add_string_reloption(bits32 kinds, const char *name, const char *desc, - const char *default_val, validate_string_relopt validator); + const char *default_val, validate_string_relopt validator, + LOCKMODE lockmode); extern Datum transformRelOptions(Datum oldOptions, List *defList, const char *namspace, char *validnsps[], -- 2.23.0
signature.asc
Description: PGP signature
