Thread: Fix bug with indexes on whole-row expressions

Fix bug with indexes on whole-row expressions

From
ywgrit
Date:
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.

I forbid to create indexes on whole-row expression in the following patch.
I'd like to hear your opinions.


--
Best Wishes,
ywgrit
Attachment

Re: Fix bug with indexes on whole-row expressions

From
Tom Lane
Date:
ywgrit <yw987194828@gmail.com> 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

From
Nikolay Samokhvalov
Date:
On Wed, Dec 13, 2023 at 7:01 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
ywgrit <yw987194828@gmail.com> 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

From
ywgrit
Date:
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 <tgl@sss.pgh.pa.us> 于2023年12月13日周三 23:01写道:
ywgrit <yw987194828@gmail.com> 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