pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #17220: ALTER INDEX ALTER COLUMN SET (..) with an optionless opclass makes index and table unusable
16+ messages / 6 participants
[nested] [flat]
* BUG #17220: ALTER INDEX ALTER COLUMN SET (..) with an optionless opclass makes index and table unusable
@ 2021-10-11 15:25 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 16+ messages in thread
From: PG Bug reporting form @ 2021-10-11 15:25 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: postgresql@zr40.nl
The following bug has been logged on the website:
Bug reference: 17220
Logged by: Matthijs van der Vleuten
Email address: postgresql@zr40.nl
PostgreSQL version: 14.0
Operating system: Debian sid
Description:
User 'musttu' on IRC reported the following bug: After running "ALTER INDEX
some_idx ALTER COLUMN expr SET (n_distinct=100)", the index and table become
unusable. All further statements involving the table result in: "ERROR:
operator class text_ops has no options".
They reported this on the RDS version of 13.3, but I've been able to
reproduce this on Debian with 13.4 and 14.0. It does not reproduce on 12.8,
all statements succeed on that version.
As a workaround, I've suggested the following catalog change in order to be
able to drop the index:
UPDATE pg_attribute SET attoptions = NULL WHERE attrelid =
'tbl_col_idx'::regclass;
However, they were not able to do this, since RDS does not expose a true
superuser.
Reproduction:
zr40@[local]:5432 ~=# select version();
version
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────
PostgreSQL 14.0 (Debian 14.0-1.pgdg+1) on x86_64-pc-linux-gnu, compiled by
gcc (Debian 10.3.0-11) 10.3.0, 64-bit
(1 row)
zr40@[local]:5432 ~=# create table test (col text);
CREATE TABLE
zr40@[local]:5432 ~=# create index on test (col);
CREATE INDEX
zr40@[local]:5432 ~=# alter index test_col_idx alter column col set
(n_distinct=100);
ALTER INDEX
zr40@[local]:5432 ~=# alter index test_col_idx alter column col reset
(n_distinct);
ERROR: 22023: operator class text_ops has no options
LOCATION: index_opclass_options, indexam.c:971
zr40@[local]:5432 ~=# drop index test_col_idx;
ERROR: 22023: operator class text_ops has no options
LOCATION: index_opclass_options, indexam.c:971
zr40@[local]:5432 ~=# drop table test;
ERROR: 22023: operator class text_ops has no options
LOCATION: index_opclass_options, indexam.c:971
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #17220: ALTER INDEX ALTER COLUMN SET (..) with an optionless opclass makes index and table unusable
@ 2021-10-11 18:03 Vik Fearing <vik@postgresfriends.org>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 16+ messages in thread
From: Vik Fearing @ 2021-10-11 18:03 UTC (permalink / raw)
To: postgresql@zr40.nl, Alexander Korotkov <aekorotkov@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
On 10/11/21 5:25 PM, PG Bug reporting form wrote:
>
> User 'musttu' on IRC reported the following bug: After running "ALTER INDEX
> some_idx ALTER COLUMN expr SET (n_distinct=100)", the index and table become
> unusable. All further statements involving the table result in: "ERROR:
> operator class text_ops has no options".
>
> They reported this on the RDS version of 13.3, but I've been able to
> reproduce this on Debian with 13.4 and 14.0. It does not reproduce on 12.8,
> all statements succeed on that version.
This was broken by 911e702077 (Implement operator class parameters).
--
Vik Fearing
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #17220: ALTER INDEX ALTER COLUMN SET (..) with an optionless opclass makes index and table unusable
@ 2021-10-13 00:06 Bossart, Nathan <bossartn@amazon.com>
parent: Vik Fearing <vik@postgresfriends.org>
0 siblings, 2 replies; 16+ messages in thread
From: Bossart, Nathan @ 2021-10-13 00:06 UTC (permalink / raw)
To: Vik Fearing <vik@postgresfriends.org>; postgresql@zr40.nl <postgresql@zr40.nl>; Alexander Korotkov <aekorotkov@gmail.com>; +Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On 10/11/21, 11:03 AM, "Vik Fearing" <vik@postgresfriends.org> wrote:
> On 10/11/21 5:25 PM, PG Bug reporting form wrote:
>>
>> User 'musttu' on IRC reported the following bug: After running "ALTER INDEX
>> some_idx ALTER COLUMN expr SET (n_distinct=100)", the index and table become
>> unusable. All further statements involving the table result in: "ERROR:
>> operator class text_ops has no options".
>>
>> They reported this on the RDS version of 13.3, but I've been able to
>> reproduce this on Debian with 13.4 and 14.0. It does not reproduce on 12.8,
>> all statements succeed on that version.
>
> This was broken by 911e702077 (Implement operator class parameters).
Moving to pgsql-hackers@.
At first glance, it looks like ALTER INDEX .. ALTER COLUMN ... SET
uses the wrong validation function. I've attached a patch where I've
attempted to fix that and added some tests.
Nathan
Attachments:
[application/octet-stream] alter_index_column_set_validation.patch (4.1K, ../../E29EF33B-CA1A-4146-BFCF-C8EC08FB91A8@amazon.com/2-alter_index_column_set_validation.patch)
download | inline diff:
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1c2ebe1bf6..f68e86467b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -8066,7 +8066,11 @@ ATExecSetOptions(Relation rel, const char *colName, Node *options,
castNode(List, options), NULL, NULL,
false, isReset);
/* Validate new options */
- (void) attribute_reloptions(newOptions, true);
+ if (rel->rd_rel->relkind == RELKIND_INDEX ||
+ rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
+ (void) index_opclass_options(rel, attnum, newOptions, true);
+ else
+ (void) attribute_reloptions(newOptions, true);
/* Build new tuple. */
memset(repl_null, false, sizeof(repl_null));
diff --git a/src/test/regress/expected/btree_index.out b/src/test/regress/expected/btree_index.out
index bc113a70b4..fc5f8b5fb1 100644
--- a/src/test/regress/expected/btree_index.out
+++ b/src/test/regress/expected/btree_index.out
@@ -329,3 +329,6 @@ INSERT INTO delete_test_table SELECT i, 1, 2, 3 FROM generate_series(1,1000) i;
-- Test unsupported btree opclass parameters
create index on btree_tall_tbl (id int4_ops(foo=1));
ERROR: operator class int4_ops has no options
+create index on btree_tall_tbl (id);
+alter index btree_tall_tbl_id_idx alter column id set (ndistinct=100);
+ERROR: operator class int4_ops has no options
diff --git a/src/test/regress/expected/tsearch.out b/src/test/regress/expected/tsearch.out
index 45b92a6338..48561d4853 100644
--- a/src/test/regress/expected/tsearch.out
+++ b/src/test/regress/expected/tsearch.out
@@ -513,6 +513,21 @@ SELECT count(*) FROM test_tsvector WHERE a @@ '!wd:D';
(1 row)
-- Test siglen parameter of GiST tsvector_ops
+CREATE INDEX wowidx1 ON test_tsvector USING gist (a);
+ALTER INDEX wowidx1 ALTER COLUMN a SET (foo=1);
+ERROR: unrecognized parameter "foo"
+ALTER INDEX wowidx1 ALTER COLUMN a SET (siglen=0);
+ERROR: value 0 out of bounds for option "siglen"
+DETAIL: Valid values are between "1" and "2024".
+ALTER INDEX wowidx1 ALTER COLUMN a SET (siglen=2048);
+ERROR: value 2048 out of bounds for option "siglen"
+DETAIL: Valid values are between "1" and "2024".
+ALTER INDEX wowidx1 ALTER COLUMN a SET (siglen=100,foo='bar');
+ERROR: unrecognized parameter "foo"
+ALTER INDEX wowidx1 ALTER COLUMN a SET (siglen=100, siglen = 200);
+ERROR: parameter "siglen" specified more than once
+ALTER INDEX wowidx1 ALTER COLUMN a SET (siglen=1);
+DROP INDEX wowidx1;
CREATE INDEX wowidx1 ON test_tsvector USING gist (a tsvector_ops(foo=1));
ERROR: unrecognized parameter "foo"
CREATE INDEX wowidx1 ON test_tsvector USING gist (a tsvector_ops(siglen=0));
diff --git a/src/test/regress/sql/btree_index.sql b/src/test/regress/sql/btree_index.sql
index c60312db2d..7552c39a01 100644
--- a/src/test/regress/sql/btree_index.sql
+++ b/src/test/regress/sql/btree_index.sql
@@ -172,3 +172,5 @@ INSERT INTO delete_test_table SELECT i, 1, 2, 3 FROM generate_series(1,1000) i;
-- Test unsupported btree opclass parameters
create index on btree_tall_tbl (id int4_ops(foo=1));
+create index on btree_tall_tbl (id);
+alter index btree_tall_tbl_id_idx alter column id set (ndistinct=100);
diff --git a/src/test/regress/sql/tsearch.sql b/src/test/regress/sql/tsearch.sql
index d929210998..96998fdcf6 100644
--- a/src/test/regress/sql/tsearch.sql
+++ b/src/test/regress/sql/tsearch.sql
@@ -130,6 +130,15 @@ SELECT count(*) FROM test_tsvector WHERE a @@ '!wd:A';
SELECT count(*) FROM test_tsvector WHERE a @@ '!wd:D';
-- Test siglen parameter of GiST tsvector_ops
+CREATE INDEX wowidx1 ON test_tsvector USING gist (a);
+ALTER INDEX wowidx1 ALTER COLUMN a SET (foo=1);
+ALTER INDEX wowidx1 ALTER COLUMN a SET (siglen=0);
+ALTER INDEX wowidx1 ALTER COLUMN a SET (siglen=2048);
+ALTER INDEX wowidx1 ALTER COLUMN a SET (siglen=100,foo='bar');
+ALTER INDEX wowidx1 ALTER COLUMN a SET (siglen=100, siglen = 200);
+ALTER INDEX wowidx1 ALTER COLUMN a SET (siglen=1);
+DROP INDEX wowidx1;
+
CREATE INDEX wowidx1 ON test_tsvector USING gist (a tsvector_ops(foo=1));
CREATE INDEX wowidx1 ON test_tsvector USING gist (a tsvector_ops(siglen=0));
CREATE INDEX wowidx1 ON test_tsvector USING gist (a tsvector_ops(siglen=2048));
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #17220: ALTER INDEX ALTER COLUMN SET (..) with an optionless opclass makes index and table unusable
@ 2021-10-13 00:30 Vik Fearing <vik@postgresfriends.org>
parent: Bossart, Nathan <bossartn@amazon.com>
1 sibling, 1 reply; 16+ messages in thread
From: Vik Fearing @ 2021-10-13 00:30 UTC (permalink / raw)
To: Bossart, Nathan <bossartn@amazon.com>; postgresql@zr40.nl <postgresql@zr40.nl>; Alexander Korotkov <aekorotkov@gmail.com>; +Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On 10/13/21 2:06 AM, Bossart, Nathan wrote:
> On 10/11/21, 11:03 AM, "Vik Fearing" <vik@postgresfriends.org> wrote:
>> On 10/11/21 5:25 PM, PG Bug reporting form wrote:
>>>
>>> User 'musttu' on IRC reported the following bug: After running "ALTER INDEX
>>> some_idx ALTER COLUMN expr SET (n_distinct=100)", the index and table become
>>> unusable. All further statements involving the table result in: "ERROR:
>>> operator class text_ops has no options".
>>>
>>> They reported this on the RDS version of 13.3, but I've been able to
>>> reproduce this on Debian with 13.4 and 14.0. It does not reproduce on 12.8,
>>> all statements succeed on that version.
>>
>> This was broken by 911e702077 (Implement operator class parameters).
>
> Moving to pgsql-hackers@.
>
> At first glance, it looks like ALTER INDEX .. ALTER COLUMN ... SET
> uses the wrong validation function. I've attached a patch where I've
> attempted to fix that and added some tests.
Ah, thank you. I was in the (slow) process of writing basically this
exact patch. So I'll stop now and endorse yours.
--
Vik Fearing
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #17220: ALTER INDEX ALTER COLUMN SET (..) with an optionless opclass makes index and table unusable
@ 2021-10-13 02:20 Bossart, Nathan <bossartn@amazon.com>
parent: Vik Fearing <vik@postgresfriends.org>
0 siblings, 0 replies; 16+ messages in thread
From: Bossart, Nathan @ 2021-10-13 02:20 UTC (permalink / raw)
To: Vik Fearing <vik@postgresfriends.org>; postgresql@zr40.nl <postgresql@zr40.nl>; Alexander Korotkov <aekorotkov@gmail.com>; +Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On 10/12/21, 5:31 PM, "Vik Fearing" <vik@postgresfriends.org> wrote:
> On 10/13/21 2:06 AM, Bossart, Nathan wrote:
>> Moving to pgsql-hackers@.
>>
>> At first glance, it looks like ALTER INDEX .. ALTER COLUMN ... SET
>> uses the wrong validation function. I've attached a patch where I've
>> attempted to fix that and added some tests.
>
> Ah, thank you. I was in the (slow) process of writing basically this
> exact patch. So I'll stop now and endorse yours.
Oops, sorry about that. Thanks for the endorsement.
Nathan
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #17220: ALTER INDEX ALTER COLUMN SET (..) with an optionless opclass makes index and table unusable
@ 2021-10-13 03:42 Michael Paquier <michael@paquier.xyz>
parent: Bossart, Nathan <bossartn@amazon.com>
1 sibling, 1 reply; 16+ messages in thread
From: Michael Paquier @ 2021-10-13 03:42 UTC (permalink / raw)
To: Bossart, Nathan <bossartn@amazon.com>; +Cc: Vik Fearing <vik@postgresfriends.org>; postgresql@zr40.nl <postgresql@zr40.nl>; Alexander Korotkov <aekorotkov@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Wed, Oct 13, 2021 at 12:06:32AM +0000, Bossart, Nathan wrote:
> At first glance, it looks like ALTER INDEX .. ALTER COLUMN ... SET
> uses the wrong validation function. I've attached a patch where I've
> attempted to fix that and added some tests.
The gap is larger than than, because ALTER INDEX .. ALTER COLUMN
.. SET is supported by the parser but we don't document it. The only
thing we document now is SET STATISTICS that applies to a column
*number*.
Anyway, specifying a column name for an ALTER INDEX is not right, no?
Just take for example the case of an expression which has a hardcoded
column name in pg_attribute. So these are not specific to indexes,
which is why we apply column numbers for the statistics case. I think
that we'd better just reject those cases until there is a proper
design done here. As far as I can see, I guess that we should do
things similarly to what we do for SET STATISTICS with column
numbers when it comes to indexes.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../YWZViGJFJkrgBKwk@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #17220: ALTER INDEX ALTER COLUMN SET (..) with an optionless opclass makes index and table unusable
@ 2021-10-13 08:29 Dilip Kumar <dilipbalaut@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 16+ messages in thread
From: Dilip Kumar @ 2021-10-13 08:29 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Bossart, Nathan <bossartn@amazon.com>; Vik Fearing <vik@postgresfriends.org>; postgresql@zr40.nl <postgresql@zr40.nl>; Alexander Korotkov <aekorotkov@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On Wed, Oct 13, 2021 at 9:12 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Oct 13, 2021 at 12:06:32AM +0000, Bossart, Nathan wrote:
> > At first glance, it looks like ALTER INDEX .. ALTER COLUMN ... SET
> > uses the wrong validation function. I've attached a patch where I've
> > attempted to fix that and added some tests.
>
> The gap is larger than than, because ALTER INDEX .. ALTER COLUMN
> .. SET is supported by the parser but we don't document it. The only
> thing we document now is SET STATISTICS that applies to a column
> *number*.
>
> Anyway, specifying a column name for an ALTER INDEX is not right, no?
> Just take for example the case of an expression which has a hardcoded
> column name in pg_attribute. So these are not specific to indexes,
> which is why we apply column numbers for the statistics case. I think
> that we'd better just reject those cases until there is a proper
> design done here. As far as I can see, I guess that we should do
> things similarly to what we do for SET STATISTICS with column
> numbers when it comes to indexes.
+1 it should behave similarly to SET STATISTICS for the index and if
someone tries to set with the column name then it should throw an
error.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #17220: ALTER INDEX ALTER COLUMN SET (..) with an optionless opclass makes index and table unusable
@ 2021-10-13 17:20 Bossart, Nathan <bossartn@amazon.com>
parent: Dilip Kumar <dilipbalaut@gmail.com>
0 siblings, 1 reply; 16+ messages in thread
From: Bossart, Nathan @ 2021-10-13 17:20 UTC (permalink / raw)
To: Dilip Kumar <dilipbalaut@gmail.com>; Michael Paquier <michael@paquier.xyz>; +Cc: Vik Fearing <vik@postgresfriends.org>; postgresql@zr40.nl <postgresql@zr40.nl>; Alexander Korotkov <aekorotkov@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
On 10/13/21, 1:31 AM, "Dilip Kumar" <dilipbalaut@gmail.com> wrote:
> On Wed, Oct 13, 2021 at 9:12 AM Michael Paquier <michael@paquier.xyz> wrote:
>> Anyway, specifying a column name for an ALTER INDEX is not right, no?
>> Just take for example the case of an expression which has a hardcoded
>> column name in pg_attribute. So these are not specific to indexes,
>> which is why we apply column numbers for the statistics case. I think
>> that we'd better just reject those cases until there is a proper
>> design done here. As far as I can see, I guess that we should do
>> things similarly to what we do for SET STATISTICS with column
>> numbers when it comes to indexes.
>
> +1 it should behave similarly to SET STATISTICS for the index and if
> someone tries to set with the column name then it should throw an
> error.
Good point. I agree that rejecting this case is probably the best
option for now. In addition to the bug mentioned in this thread, this
functionality doesn't even work for supported options currently.
postgres=> create table test (a tsvector);
CREATE TABLE
postgres=> create index on test using gist (a);
CREATE INDEX
postgres=> alter index test_a_idx alter column a set (siglen = 100);
ERROR: unrecognized parameter "siglen"
AFAICT the fact that these commands can succeed at all seems to be
unintentional, and I wonder if modifying these options requires extra
steps such as rebuilding the index.
I've attached new patch that just adds an ERROR.
Nathan
Attachments:
[application/octet-stream] disallow_alter_index_column_set.patch (1.9K, ../../678CF71A-934B-4FD0-BF63-E41D795E41C9@amazon.com/2-disallow_alter_index_column_set.patch)
download | inline diff:
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1c2ebe1bf6..eb3c146f25 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -8041,6 +8041,12 @@ ATExecSetOptions(Relation rel, const char *colName, Node *options,
bool repl_null[Natts_pg_attribute];
bool repl_repl[Natts_pg_attribute];
+ if (rel->rd_rel->relkind == RELKIND_INDEX ||
+ rel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot alter options for index column")));
+
attrelation = table_open(AttributeRelationId, RowExclusiveLock);
tuple = SearchSysCacheAttName(RelationGetRelid(rel), colName);
diff --git a/src/test/regress/expected/btree_index.out b/src/test/regress/expected/btree_index.out
index bc113a70b4..f80303ae3d 100644
--- a/src/test/regress/expected/btree_index.out
+++ b/src/test/regress/expected/btree_index.out
@@ -329,3 +329,7 @@ INSERT INTO delete_test_table SELECT i, 1, 2, 3 FROM generate_series(1,1000) i;
-- Test unsupported btree opclass parameters
create index on btree_tall_tbl (id int4_ops(foo=1));
ERROR: operator class int4_ops has no options
+create index on btree_tall_tbl (id);
+alter index btree_tall_tbl_id_idx alter column id set (n_distinct=100);
+ERROR: cannot alter options for index column
+drop index btree_tall_tbl_id_idx;
diff --git a/src/test/regress/sql/btree_index.sql b/src/test/regress/sql/btree_index.sql
index c60312db2d..3519d80696 100644
--- a/src/test/regress/sql/btree_index.sql
+++ b/src/test/regress/sql/btree_index.sql
@@ -172,3 +172,7 @@ INSERT INTO delete_test_table SELECT i, 1, 2, 3 FROM generate_series(1,1000) i;
-- Test unsupported btree opclass parameters
create index on btree_tall_tbl (id int4_ops(foo=1));
+
+create index on btree_tall_tbl (id);
+alter index btree_tall_tbl_id_idx alter column id set (n_distinct=100);
+drop index btree_tall_tbl_id_idx;
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #17220: ALTER INDEX ALTER COLUMN SET (..) with an optionless opclass makes index and table unusable
@ 2021-10-14 02:07 Michael Paquier <michael@paquier.xyz>
parent: Bossart, Nathan <bossartn@amazon.com>
0 siblings, 1 reply; 16+ messages in thread
From: Michael Paquier @ 2021-10-14 02:07 UTC (permalink / raw)
To: Bossart, Nathan <bossartn@amazon.com>; +Cc: Dilip Kumar <dilipbalaut@gmail.com>; Vik Fearing <vik@postgresfriends.org>; postgresql@zr40.nl <postgresql@zr40.nl>; Alexander Korotkov <aekorotkov@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>; Robert Haas <robertmhaas@gmail.com>
On Wed, Oct 13, 2021 at 05:20:56PM +0000, Bossart, Nathan wrote:
> AFAICT the fact that these commands can succeed at all seems to be
> unintentional, and I wonder if modifying these options requires extra
> steps such as rebuilding the index.
I was looking at all this business with more attention, and this code
block is standing out in analyze.c:
/*
* Now we can compute the statistics for the expression columns.
*/
if (numindexrows > 0)
{
MemoryContextSwitchTo(col_context);
for (i = 0; i < attr_cnt; i++)
{
VacAttrStats *stats = thisdata->vacattrstats[i];
AttributeOpts *aopt =
get_attribute_options(stats->attr->attrelid,
stats->attr->attnum);
stats->exprvals = exprvals + i;
stats->exprnulls = exprnulls + i;
stats->rowstride = attr_cnt;
stats->compute_stats(stats,
ind_fetch_func,
numindexrows,
totalindexrows);
/*
* If the n_distinct option is specified, it overrides the
* above computation. For indices, we always use just
* n_distinct, not n_distinct_inherited.
*/
if (aopt != NULL && aopt->n_distinct != 0.0)
stats->stadistinct = aopt->n_distinct;
MemoryContextResetAndDeleteChildren(col_context);
}
}
When computing statistics on an index expression, this code means that
we would grab the value of n_distinct from the *index* if set and
force the stats to use it, and not use what the parent table has. For
example, say:
create table aa (a int);
insert into aa values (generate_series(1,1000));
create index aai on aa((a+a)) where a > 500;
alter index aai alter column expr set (n_distinct = 2);
analyze aa; -- n_distinct forced to 2.0 for the index stats
This code comes from 76a47c0 back in 2010. In PG <= 12, this would
work, but that does not as of 13~. Enforcing n_distinct for index
attributes was discussed back when this code was introduced:
https://www.postgresql.org/message-id/603c8f071001101127w3253899vb3f3e15073638774@mail.gmail.com
This means that we've lost the ability to enforce n_distinct for
expression indexes for two years. But, do we really care about this
case? My answer to that would be "no" as long as we don't have a
documented grammar rather, and we don't dump them either. But I think
that we'd better do something with the code in analyze.c rather than
letting it just dead, and my take is that we should remove the call to
get_attribute_options() for this code path.
Any opinions? @Robert: you were involved in 76a47c0, so I am adding
you in CC.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../YWeQ2UXvXUH%2FGt4T@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #17220: ALTER INDEX ALTER COLUMN SET (..) with an optionless opclass makes index and table unusable
@ 2021-10-18 07:46 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 2 replies; 16+ messages in thread
From: Michael Paquier @ 2021-10-18 07:46 UTC (permalink / raw)
To: Bossart, Nathan <bossartn@amazon.com>; +Cc: Dilip Kumar <dilipbalaut@gmail.com>; Vik Fearing <vik@postgresfriends.org>; postgresql@zr40.nl <postgresql@zr40.nl>; Alexander Korotkov <aekorotkov@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>; Robert Haas <robertmhaas@gmail.com>
On Thu, Oct 14, 2021 at 11:07:21AM +0900, Michael Paquier wrote:
> This means that we've lost the ability to enforce n_distinct for
> expression indexes for two years. But, do we really care about this
> case? My answer to that would be "no" as long as we don't have a
> documented grammar rather, and we don't dump them either. But I think
> that we'd better do something with the code in analyze.c rather than
> letting it just dead, and my take is that we should remove the call to
> get_attribute_options() for this code path.
>
> Any opinions? @Robert: you were involved in 76a47c0, so I am adding
> you in CC.
Hearing nothing, and after pondering on this point, I think that
removing the get_attribute_options() is the right way to go for now
if there is a point in the future to get n_distinct done for all index
AMs.
I have reviewed the last patch posted upthread, and while testing
partitioned indexes I have noticed that we don't need to do a custom
check as part of ATExecSetOptions(), because we have already that in
ATSimplePermissions() with details on the relkind failing. This makes
the patch simpler, with a better error message generated. I have
added a case for partitioned indexes while on it.
Worth noting that I have spotted an extra weird call of
get_attribute_options() in extended statistics. This is unrelated to
this thread and I am not done analyzing it yet, but a quick check
shows that we call it with an InvalidOid for expression stats, which
is surprising.. I'll start a thread once/if I find anything
interesting on this one.
Attached is the patch I am finishing with, that should go down to
v13 (this is going to conflict on REL_13_STABLE, for sure).
Thoughts?
--
Michael
Attachments:
[text/x-diff] disallow_alter_index_column_set_v2.patch (3.9K, ../../YW0mYDzpVhEjcbNp@paquier.xyz/2-disallow_alter_index_column_set_v2.patch)
download | inline diff:
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 8bfb2ad958..4928702aec 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -965,9 +965,6 @@ compute_index_stats(Relation onerel, double totalrows,
for (i = 0; i < attr_cnt; i++)
{
VacAttrStats *stats = thisdata->vacattrstats[i];
- AttributeOpts *aopt =
- get_attribute_options(stats->attr->attrelid,
- stats->attr->attnum);
stats->exprvals = exprvals + i;
stats->exprnulls = exprnulls + i;
@@ -977,14 +974,6 @@ compute_index_stats(Relation onerel, double totalrows,
numindexrows,
totalindexrows);
- /*
- * If the n_distinct option is specified, it overrides the
- * above computation. For indices, we always use just
- * n_distinct, not n_distinct_inherited.
- */
- if (aopt != NULL && aopt->n_distinct != 0.0)
- stats->stadistinct = aopt->n_distinct;
-
MemoryContextResetAndDeleteChildren(col_context);
}
}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1c2ebe1bf6..defd0b31d5 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4494,7 +4494,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
break;
case AT_SetOptions: /* ALTER COLUMN SET ( options ) */
case AT_ResetOptions: /* ALTER COLUMN RESET ( options ) */
- ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_MATVIEW | ATT_INDEX | ATT_FOREIGN_TABLE);
+ ATSimplePermissions(cmd->subtype, rel, ATT_TABLE | ATT_MATVIEW | ATT_FOREIGN_TABLE);
/* This command never recurses */
pass = AT_PASS_MISC;
break;
diff --git a/src/test/regress/expected/btree_index.out b/src/test/regress/expected/btree_index.out
index bc113a70b4..cd566cfb6d 100644
--- a/src/test/regress/expected/btree_index.out
+++ b/src/test/regress/expected/btree_index.out
@@ -329,3 +329,18 @@ INSERT INTO delete_test_table SELECT i, 1, 2, 3 FROM generate_series(1,1000) i;
-- Test unsupported btree opclass parameters
create index on btree_tall_tbl (id int4_ops(foo=1));
ERROR: operator class int4_ops has no options
+-- Test case of ALTER INDEX with abuse of column names for indexes.
+-- This grammar is not officially supported, but the parser allows it.
+CREATE INDEX btree_tall_idx2 ON btree_tall_tbl (id);
+ALTER INDEX btree_tall_idx2 ALTER COLUMN id SET (n_distinct=100);
+ERROR: ALTER action ALTER COLUMN ... SET cannot be performed on relation "btree_tall_idx2"
+DETAIL: This operation is not supported for indexes.
+DROP INDEX btree_tall_tbl_idx2;
+ERROR: index "btree_tall_tbl_idx2" does not exist
+-- Partitioned table
+CREATE TABLE btree_part (id int4) PARTITION BY RANGE (id);
+CREATE INDEX btree_part_idx ON btree_part(id);
+ALTER INDEX btree_part_idx ALTER COLUMN id SET (n_distinct=100);
+ERROR: ALTER action ALTER COLUMN ... SET cannot be performed on relation "btree_part_idx"
+DETAIL: This operation is not supported for partitioned indexes.
+DROP TABLE btree_part;
diff --git a/src/test/regress/sql/btree_index.sql b/src/test/regress/sql/btree_index.sql
index c60312db2d..dc9046e06f 100644
--- a/src/test/regress/sql/btree_index.sql
+++ b/src/test/regress/sql/btree_index.sql
@@ -172,3 +172,14 @@ INSERT INTO delete_test_table SELECT i, 1, 2, 3 FROM generate_series(1,1000) i;
-- Test unsupported btree opclass parameters
create index on btree_tall_tbl (id int4_ops(foo=1));
+
+-- Test case of ALTER INDEX with abuse of column names for indexes.
+-- This grammar is not officially supported, but the parser allows it.
+CREATE INDEX btree_tall_idx2 ON btree_tall_tbl (id);
+ALTER INDEX btree_tall_idx2 ALTER COLUMN id SET (n_distinct=100);
+DROP INDEX btree_tall_tbl_idx2;
+-- Partitioned table
+CREATE TABLE btree_part (id int4) PARTITION BY RANGE (id);
+CREATE INDEX btree_part_idx ON btree_part(id);
+ALTER INDEX btree_part_idx ALTER COLUMN id SET (n_distinct=100);
+DROP TABLE btree_part;
[application/pgp-signature] signature.asc (832B, ../../YW0mYDzpVhEjcbNp@paquier.xyz/3-signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #17220: ALTER INDEX ALTER COLUMN SET (..) with an optionless opclass makes index and table unusable
@ 2021-10-18 21:43 Bossart, Nathan <bossartn@amazon.com>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 16+ messages in thread
From: Bossart, Nathan @ 2021-10-18 21:43 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Dilip Kumar <dilipbalaut@gmail.com>; Vik Fearing <vik@postgresfriends.org>; postgresql@zr40.nl <postgresql@zr40.nl>; Alexander Korotkov <aekorotkov@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>; Robert Haas <robertmhaas@gmail.com>
On 10/18/21, 12:47 AM, "Michael Paquier" <michael@paquier.xyz> wrote:
> I have reviewed the last patch posted upthread, and while testing
> partitioned indexes I have noticed that we don't need to do a custom
> check as part of ATExecSetOptions(), because we have already that in
> ATSimplePermissions() with details on the relkind failing. This makes
> the patch simpler, with a better error message generated. I have
> added a case for partitioned indexes while on it.
Ah, yes, that is much better.
> Attached is the patch I am finishing with, that should go down to
> v13 (this is going to conflict on REL_13_STABLE, for sure).
+DROP INDEX btree_tall_tbl_idx2;
+ERROR: index "btree_tall_tbl_idx2" does not exist
I think this is supposed to be "btree_tall_idx2". Otherwise, the
patch looks reasonable to me.
Nathan
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #17220: ALTER INDEX ALTER COLUMN SET (..) with an optionless opclass makes index and table unusable
@ 2021-10-19 02:08 Michael Paquier <michael@paquier.xyz>
parent: Bossart, Nathan <bossartn@amazon.com>
0 siblings, 1 reply; 16+ messages in thread
From: Michael Paquier @ 2021-10-19 02:08 UTC (permalink / raw)
To: Bossart, Nathan <bossartn@amazon.com>; +Cc: Dilip Kumar <dilipbalaut@gmail.com>; Vik Fearing <vik@postgresfriends.org>; postgresql@zr40.nl <postgresql@zr40.nl>; Alexander Korotkov <aekorotkov@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>; Robert Haas <robertmhaas@gmail.com>
On Mon, Oct 18, 2021 at 09:43:58PM +0000, Bossart, Nathan wrote:
> +DROP INDEX btree_tall_tbl_idx2;
> +ERROR: index "btree_tall_tbl_idx2" does not exist
>
> I think this is supposed to be "btree_tall_idx2". Otherwise, the
> patch looks reasonable to me.
Thanks for double-checking. Applied and back-patched, with a small
conflict regarding the error message in ~14.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../YW4ohuwDEvCTcVz+@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #17220: ALTER INDEX ALTER COLUMN SET (..) with an optionless opclass makes index and table unusable
@ 2021-10-19 03:07 Bossart, Nathan <bossartn@amazon.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 16+ messages in thread
From: Bossart, Nathan @ 2021-10-19 03:07 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Dilip Kumar <dilipbalaut@gmail.com>; Vik Fearing <vik@postgresfriends.org>; postgresql@zr40.nl <postgresql@zr40.nl>; Alexander Korotkov <aekorotkov@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>; Robert Haas <robertmhaas@gmail.com>
On 10/18/21, 7:09 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> Thanks for double-checking. Applied and back-patched, with a small
> conflict regarding the error message in ~14.
Thanks!
Nathan
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #17220: ALTER INDEX ALTER COLUMN SET (..) with an optionless opclass makes index and table unusable
@ 2021-10-19 06:48 Matthijs van der Vleuten <postgresql@zr40.nl>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 16+ messages in thread
From: Matthijs van der Vleuten @ 2021-10-19 06:48 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; Bossart, Nathan <bossartn@amazon.com>; +Cc: Dilip Kumar <dilipbalaut@gmail.com>; Vik Fearing <vik@postgresfriends.org>; Alexander Korotkov <aekorotkov@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>; Robert Haas <robertmhaas@gmail.com>
On Mon, Oct 18, 2021, at 09:46, Michael Paquier wrote:
> On Thu, Oct 14, 2021 at 11:07:21AM +0900, Michael Paquier wrote:
> Attached is the patch I am finishing with, that should go down to
> v13 (this is going to conflict on REL_13_STABLE, for sure).
>
> Thoughts?
The test case doesn't seem entirely correct to me? The index being dropped (btree_tall_tbl_idx2) doesn't exist.
Also, I don't believe this tests the case of dropping the index when it previously has been altered in this way.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #17220: ALTER INDEX ALTER COLUMN SET (..) with an optionless opclass makes index and table unusable
@ 2021-10-19 14:40 Bossart, Nathan <bossartn@amazon.com>
parent: Matthijs van der Vleuten <postgresql@zr40.nl>
0 siblings, 1 reply; 16+ messages in thread
From: Bossart, Nathan @ 2021-10-19 14:40 UTC (permalink / raw)
To: Matthijs van der Vleuten <postgresql@zr40.nl>; Michael Paquier <michael@paquier.xyz>; +Cc: Dilip Kumar <dilipbalaut@gmail.com>; Vik Fearing <vik@postgresfriends.org>; Alexander Korotkov <aekorotkov@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>; Robert Haas <robertmhaas@gmail.com>
On 10/18/21, 11:49 PM, "Matthijs van der Vleuten" <postgresql@zr40.nl> wrote:
> The test case doesn't seem entirely correct to me? The index being
> dropped (btree_tall_tbl_idx2) doesn't exist.
This was fixed before it was committed [0].
> Also, I don't believe this tests the case of dropping the index when
> it previously has been altered in this way.
That can still fail with the "has no options" ERROR, and fixing it
will still require a manual catalog update. The ERROR is actually
coming from the call to index_open(), so bypassing it might require
some rather intrusive changes. Given that it took over a year for
this bug to be reported, I suspect it might be more trouble than it's
worth.
Nathan
[0] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=fdd8857
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: BUG #17220: ALTER INDEX ALTER COLUMN SET (..) with an optionless opclass makes index and table unusable
@ 2021-10-20 03:47 Michael Paquier <michael@paquier.xyz>
parent: Bossart, Nathan <bossartn@amazon.com>
0 siblings, 0 replies; 16+ messages in thread
From: Michael Paquier @ 2021-10-20 03:47 UTC (permalink / raw)
To: Bossart, Nathan <bossartn@amazon.com>; +Cc: Matthijs van der Vleuten <postgresql@zr40.nl>; Dilip Kumar <dilipbalaut@gmail.com>; Vik Fearing <vik@postgresfriends.org>; Alexander Korotkov <aekorotkov@gmail.com>; pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>; Robert Haas <robertmhaas@gmail.com>
On Tue, Oct 19, 2021 at 02:40:04PM +0000, Bossart, Nathan wrote:
> On 10/18/21, 11:49 PM, "Matthijs van der Vleuten" <postgresql@zr40.nl> wrote:
>> The test case doesn't seem entirely correct to me? The index being
>> dropped (btree_tall_tbl_idx2) doesn't exist.
>
> This was fixed before it was committed [0].
Yes, my apologies about this brain fade. The committed code is
hopefully fine :)
>> Also, I don't believe this tests the case of dropping the index when
>> it previously has been altered in this way.
>
> That can still fail with the "has no options" ERROR, and fixing it
> will still require a manual catalog update. The ERROR is actually
> coming from the call to index_open(), so bypassing it might require
> some rather intrusive changes. Given that it took over a year for
> this bug to be reported, I suspect it might be more trouble than it's
> worth.
This may need a mention in the release notes, but the problem is I
guess not that spread enough to worry or people would have complained
more since 13 was out. Logical dumps discard that automatically and
even for the ANALYZE case, the pre-committed code would have just
ignored the reloptions retrieved by get_attribute_options().
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../YW+ROCXCtFshHvDx@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 16+ messages in thread
end of thread, other threads:[~2021-10-20 03:47 UTC | newest]
Thread overview: 16+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-10-11 15:25 BUG #17220: ALTER INDEX ALTER COLUMN SET (..) with an optionless opclass makes index and table unusable PG Bug reporting form <noreply@postgresql.org>
2021-10-11 18:03 ` Vik Fearing <vik@postgresfriends.org>
2021-10-13 00:06 ` Bossart, Nathan <bossartn@amazon.com>
2021-10-13 00:30 ` Vik Fearing <vik@postgresfriends.org>
2021-10-13 02:20 ` Bossart, Nathan <bossartn@amazon.com>
2021-10-13 03:42 ` Michael Paquier <michael@paquier.xyz>
2021-10-13 08:29 ` Dilip Kumar <dilipbalaut@gmail.com>
2021-10-13 17:20 ` Bossart, Nathan <bossartn@amazon.com>
2021-10-14 02:07 ` Michael Paquier <michael@paquier.xyz>
2021-10-18 07:46 ` Michael Paquier <michael@paquier.xyz>
2021-10-18 21:43 ` Bossart, Nathan <bossartn@amazon.com>
2021-10-19 02:08 ` Michael Paquier <michael@paquier.xyz>
2021-10-19 03:07 ` Bossart, Nathan <bossartn@amazon.com>
2021-10-19 06:48 ` Matthijs van der Vleuten <postgresql@zr40.nl>
2021-10-19 14:40 ` Bossart, Nathan <bossartn@amazon.com>
2021-10-20 03:47 ` Michael Paquier <michael@paquier.xyz>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox