Hi all, This is a new thread related to the bug analyzed here: https://www.postgresql.org/message-id/20190919083203.gc21...@paquier.xyz
And in short, if you attempt to do an ALTER TABLE with a custom reloptions the command burns itself, like that for example this sequence: create extension bloom; create table aa (a int); create index aa_bloom ON aa USING bloom (a); alter index aa_bloom set (length = 20); Which results in the following error: ERROR: XX000: unrecognized lock mode: 2139062143 LOCATION: LockAcquireExtended, lock.c:756 The root of the problem is that the set of relation options loaded finds properly the custom options set when looking for the lock mode to use in AlterTableGetRelOptionsLockLevel(), but we never set the lock mode this option should use when allocating it, resulting in a failure. The current set of APIs does not allow either to set the lock mode associated with a custom reloption. Hence attached is a patch set to address those issues: - 0001 makes sure that any existing module creating a custom reloption has the lock mode set to AccessExclusiveMode, which would be a sane default anyway. I think that we should just back-patch that and close any holes. - 0002 is a patch which we could use to extend the existing reloption APIs to set the lock mode used. I am aware of the recent work done by Nikolay in CC to rework this API set, but I am unsure where we are going there, and the resulting patch is actually very short, being 20-line long with the current infrastructure. That could go into HEAD. Table AMs have been added in v12 so custom reloptions could gain more in popularity, but as we are very close to the release it would not be cool to break those APIs. The patch simplicity could also be a reason sufficient for a back-patch, and I don't think that there are many users of them yet. My take would be to use 0001 on all branches (or I am missing something related to custom relopts manipulation?), and consider 0002 on HEAD. Thoughts? -- Michael
From af2541d08965369f1b1fa4d8a0750e798420b3a5 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Fri, 20 Sep 2019 10:10:18 +0900 Subject: [PATCH 1/2] Fix failure for lock mode used for custom relation options --- 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 8d3f95edea77ea71eeade8e2a977e8404573665f Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Fri, 20 Sep 2019 10:17:54 +0900
Subject: [PATCH 2/2] Allow definition of lock mode for custom reloptions
---
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
