Re: Fix bug with indexes on whole-row expressions
Thanks, tom. Considering the scenario where the indexed column is a function Var on a whole expression, it's really not a good idea to disable creating index on whole expression. I tried find_composite_type_dependencies, it seems that this function can only detect dependencies created by statements such as 'CREATE INDEX test_tbl1_idx ON test_tbl1((row(x,y)::test_type1))', and cannot detect dependencies created by statements such as 'CREATE INDEX test_tbl1_idx ON test_tbl1((test _tbl1))'. After the execution of the former sql statement, 4 rows are added to the pg_depend table, one of which is the index -> pg_type dependency. After the latter sql statement is executed, only one row is added to the pg_depend table, and there is no index -> pg_type dependency, so I guess this function doesn't detect all cases of index on whole-row expression. And I would suggest to do the detection when the index is created, because then we can get the details of the index and give a warning in the way you mentioned. Tom Lane 于2023年12月13日周三 23:01写道： > ywgrit writes: > > I forbid to create indexes on whole-row expression in the following > patch. > > I'd like to hear your opinions. > > As I said in the previous thread, I don't think this can possibly > be acceptable. Surely there are people depending on the capability. > I'm not worried so much about the exact case of an index column > being a whole-row Var --- I agree that that's pretty useless --- > but an index column that is a function on a whole-row Var seems > quite useful. (Your patch fails to detect that, BTW, which means > it does not block the case presented in bug #18244.) > > I thought about extending the ALTER TABLE logic to disallow changes > in composite types that appear in index expressions. We already have > find_composite_type_dependencies(), and it turns out that this already > blocks ALTER for the case you want to forbid, but we concluded that we > didn't need to prevent it for the bug #18244 case: > > * If objsubid identifies a specific column, refer to that in error > * messages. Otherwise, search to see if there's a user column of > the > * type. (We assume system columns are never of interesting > types.) > * The search is needed because an index containing an expression > * column of the target type will just be recorded as a > whole-relation > * dependency. If we do not find a column of the type, the > dependency > * must indicate that the type is transiently referenced in an > index > * expression but not stored on disk, which we assume is OK, just > as > * we do for references in views. (It could also be that the > target > * type is embedded in some container type that is stored in an > index > * column, but the previous recursion should catch such cases.) > > Perhaps a reasonable answer would be to issue a WARNING (not error) > in the case where an index has this kind of dependency. The index > might need to be reindexed --- but it might not, too, and in any case > I doubt that flat-out forbidding the ALTER is a helpful idea. > > regards, tom lane >
Re: Fix bug with indexes on whole-row expressions
On Wed, Dec 13, 2023 at 7:01 AM Tom Lane wrote: > ywgrit writes: > > I forbid to create indexes on whole-row expression in the following > patch. > > I'd like to hear your opinions. > > As I said in the previous thread, I don't think this can possibly > be acceptable. Surely there are people depending on the capability. > I'm not worried so much about the exact case of an index column > being a whole-row Var --- I agree that that's pretty useless --- > but an index column that is a function on a whole-row Var seems > quite useful. (Your patch fails to detect that, BTW, which means > it does not block the case presented in bug #18244.) > > I thought about extending the ALTER TABLE logic to disallow changes > in composite types that appear in index expressions. We already have > find_composite_type_dependencies(), and it turns out that this already > blocks ALTER for the case you want to forbid, but we concluded that we > didn't need to prevent it for the bug #18244 case: > > * If objsubid identifies a specific column, refer to that in error > * messages. Otherwise, search to see if there's a user column of > the > * type. (We assume system columns are never of interesting > types.) > * The search is needed because an index containing an expression > * column of the target type will just be recorded as a > whole-relation > * dependency. If we do not find a column of the type, the > dependency > * must indicate that the type is transiently referenced in an > index > * expression but not stored on disk, which we assume is OK, just > as > * we do for references in views. (It could also be that the > target > * type is embedded in some container type that is stored in an > index > * column, but the previous recursion should catch such cases.) > > Perhaps a reasonable answer would be to issue a WARNING (not error) > in the case where an index has this kind of dependency. The index > might need to be reindexed --- but it might not, too, and in any case > I doubt that flat-out forbidding the ALTER is a helpful idea. > > regards, tom lane > WARNING can be easily overlooked. Users of mobile/web apps don't see Postgres WARNINGs. Forbidding ALTER sounds more reasonable. Do you see any good use cases for whole-row indexes? And for such cases, wouldn't it be reasonable for users to specify all columns explicitly? E.g.: create index on t using btree(row(c1, c2, c3));
Re: Fix bug with indexes on whole-row expressions
ywgrit writes: > I forbid to create indexes on whole-row expression in the following patch. > I'd like to hear your opinions. As I said in the previous thread, I don't think this can possibly be acceptable. Surely there are people depending on the capability. I'm not worried so much about the exact case of an index column being a whole-row Var --- I agree that that's pretty useless --- but an index column that is a function on a whole-row Var seems quite useful. (Your patch fails to detect that, BTW, which means it does not block the case presented in bug #18244.) I thought about extending the ALTER TABLE logic to disallow changes in composite types that appear in index expressions. We already have find_composite_type_dependencies(), and it turns out that this already blocks ALTER for the case you want to forbid, but we concluded that we didn't need to prevent it for the bug #18244 case: * If objsubid identifies a specific column, refer to that in error * messages. Otherwise, search to see if there's a user column of the * type. (We assume system columns are never of interesting types.) * The search is needed because an index containing an expression * column of the target type will just be recorded as a whole-relation * dependency. If we do not find a column of the type, the dependency * must indicate that the type is transiently referenced in an index * expression but not stored on disk, which we assume is OK, just as * we do for references in views. (It could also be that the target * type is embedded in some container type that is stored in an index * column, but the previous recursion should catch such cases.) Perhaps a reasonable answer would be to issue a WARNING (not error) in the case where an index has this kind of dependency. The index might need to be reindexed --- but it might not, too, and in any case I doubt that flat-out forbidding the ALTER is a helpful idea. regards, tom lane
Fix bug with indexes on whole-row expressions
Hi, Thomas Munro and Laurenz Albe.
Since I didn't subscribe to the psql-hackers mailing list before this bug
was raised, please forgive me for not being able to reply to this email by
placing the email message below.
https://www.postgresql.org/message-id/flat/e48a5d9a2d3d72985d61ee254314f5f5f5444a55.ca...@cybertec.at
I forbid to create indexes on whole-row expression in the following patch.
I'd like to hear your opinions.
--
Best Wishes,
ywgrit
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index cd23ab3b25..e4451b1d36 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -572,9 +572,18 @@ DefineIndex(Oid relationId,
Oid root_save_userid;
int root_save_sec_context;
int root_save_nestlevel;
+ ListCell *lc;
root_save_nestlevel = NewGUCNestLevel();
+ foreach (lc, stmt->indexParams)
+ {
+ IndexElem *ielem = castNode(IndexElem, lfirst(lc));
+ if (IsA(ielem->expr, Var) && castNode(Var, ielem->expr)->varattno == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("cannot create index on whole-row expression of table '%s'", ielem->indexcolname)));
+ }
/*
* Some callers need us to run with an empty default_tablespace; this is a
* necessary hack to be able to reproduce catalog state accurately when
