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
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;
signature.asc
Description: PGP signature
