Обсуждение: index on function confuses drop table cascade on child

Checkout from HEAD this morning, no modifications.  Did make
distclean and fresh build to ensure no problems caused by mixed
build.  32 bit kubuntu on single drive dual core workstation.  Fresh
initdb.  Default configuration.

postgres=# create database bug;
CREATE DATABASE
postgres=# \c bug
You are now connected to database "bug" as user "kgrittn".
bug=# create table person (namel text not null);
CREATE TABLE
bug=# create table t (id int primary key) inherits (person);
NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index
"t_pkey" for table "t"
CREATE TABLE
bug=# create or replace function sname(person) returns text
bug-#   language sql immutable as $$select upper($1.namel)$$;
CREATE FUNCTION
bug=# create index t_sname on t (sname(t));
CREATE INDEX
bug=# select oid, relname from pg_class
bug-#   where relname in ('person','t');
  oid  | relname
-------+---------
 16385 | person
 16391 | t
(2 rows)

bug=# drop table t cascade;
DROP TABLE

[so far, behavior is as expected]

bug=# drop table person cascade;
NOTICE:  drop cascades to 2 other objects
DETAIL:  drop cascades to function sname(person)
drop cascades to index t_sname
ERROR:  could not open relation with OID 16391
bug=# create table person (namel text not null);
ERROR:  relation "person" already exists
bug=# create table t (id int primary key) inherits (person);
NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index
"t_pkey" for table "t"
CREATE TABLE
bug=# create or replace function sname(person) returns text
bug-#   language sql immutable as $$select upper($1.namel)$$;
CREATE FUNCTION
bug=# create index t_sname on t (sname(t));
ERROR:  relation "t_sname" already exists
bug=# select oid, relname from pg_class
bug-#   where relname in ('person','t');
  oid  | relname
-------+---------
 16385 | person
 16401 | t
(2 rows)

All is fine if the t_searchname index is left out or the t table
defines all columns directly rather than inheriting them from
person.  Similar failure if person is dropped first or on the same
statement as t.

-Kevin

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
> create index t_sname on t (sname(t));

Huh, interesting.  The reason the DROP misbehaves is that the index
doesn't have any dependency at all on table "t".  Which appears to
be exposing the folly of this bit in find_expr_references_walker:

        /*
         * A whole-row Var references no specific columns, so adds no new
         * dependency.
         */
        if (var->varattno == InvalidAttrNumber)
            return false;

This is broken at least as far back as 8.1.  Surprising no one's noticed
before.

            regards, tom lane

I wrote:
> "Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
>> create index t_sname on t (sname(t));

> Huh, interesting.  The reason the DROP misbehaves is that the index
> doesn't have any dependency at all on table "t".  Which appears to
> be exposing the folly of this bit in find_expr_references_walker:

>         /*
>          * A whole-row Var references no specific columns, so adds no new
>          * dependency.
>          */

Hmm.  Actually there is more here than meets the eye.  There are at
least three ways we could fix this, each with their own pluses and
minuses:

1. The proximate reason that no dependency on table "t" gets generated
is that this bit in index_create() supposes that any Var in the index
expressions will result in some dependency on the index, so it need not
add a redundant one:

            /*
             * It's possible for an index to not depend on any columns of the
             * table at all, in which case we need to give it a dependency on
             * the table as a whole; else it won't get dropped when the table
             * is dropped.  This edge case is not totally useless; for
             * example, a unique index on a constant expression can serve to
             * prevent a table from containing more than one row.
             */
            if (!have_simple_col &&
             !contain_vars_of_level((Node *) indexInfo->ii_Expressions, 0) &&
                !contain_vars_of_level((Node *) indexInfo->ii_Predicate, 0))
            {
                referenced.classId = RelationRelationId;
                referenced.objectId = heapRelationId;
                referenced.objectSubId = 0;

                recordDependencyOn(&myself, &referenced, DEPENDENCY_AUTO);
            }

Of course this isn't allowing for the special case about whole-row Vars.
So one fix would be to just delete these contain_vars_of_level() checks
and thus emit a whole-table dependency record whenever there aren't any
simple index columns.  There is much to be said for this method; it's
both simpler and more robust than the current approach, and it has very
little risk of breaking anything because it won't affect any other
dependency-creating behavior.  I don't care that much about the
possibility of emitting an extra dependency record, either.  However,
this only fixes the issue for indexes, and doesn't do anything against
the possibility of current or future bugs of the same ilk elsewhere.

2. We could change the special case for whole-row Vars in
find_expr_references_walker.  Now the interesting thing about that is
that it has cases for whole-row Vars referencing either simple relations
or JOIN expressions.  Emitting a whole-table dependency seems to be the
right thing for the simple-relation case, but it is not possible to do
that for a JOIN.  You might think that we should fix the JOIN case to
generate dependencies on all the individual columns of the JOIN, but in
fact that would be exactly the wrong thing --- the whole point here is
that the whole-row reference isn't invalidated by dropping any one
column in the input relation(s).  That's why the special case is written
the way it is.  It's okay as-is for references inside ordinary query
trees, because there will be whole-row references associated with the
rtable entries anyway.  So we could possibly leave the JOIN case alone
(ie, ignore whole-row references) and just change the simple-rel case.
But that's likely to look broken no matter how much we try to explain it
in the comments; and perhaps it would actually *be* broken in some
future usage.

3. Or, perhaps we could change recordDependencyOnSingleRelExpr so that
it generates a whole-table dependency on the target relation even if
there are no Vars in the expression.  This would make it act much more
like the regular-query context that find_expr_references_walker is
expecting --- in essence, since we're fabricating a single-element
rtable for find_expr_references_walker to work with, we should fabricate
the implied whole-table dependency entry too.  But that seems a bit
weird too, and in particular it's not obvious whether to do that if in
fact the expression is empty, or doesn't contain any Var at all.


Right now, the only uses of recordDependencyOnSingleRelExpr are the ones
for index expressions and predicates (ie, exactly the current issue) and
the one for CHECK constraint expressions.  The latter case does not have
a bug because CreateConstraintEntry is coded to create a whole-table
dependency if there aren't any easily-identifiable column dependencies
(ie, exactly the equivalent of fix #1 above for the index code).

I'm a bit tempted to go with solution #1, but we'd have to recognize
that probably every future use of recordDependencyOnSingleRelExpr would
be exposed to this same type of bug if it got too cute about eliminating
"redundant" dependencies.  But at the same time, predicting what behavior
such uses might need is a tough game in itself, and maybe one that we
shouldn't get into now.

Any thoughts out there?

            regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:
> Any thoughts out there?

Color me slow, but I don't understand what allows an index creation on a
table to not systematically add a dependency entry for the index that
references the table.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

Tom Lane <tgl@sss.pgh.pa.us> wrote:

> 3. Or, perhaps we could change recordDependencyOnSingleRelExpr so
> that it generates a whole-table dependency on the target relation
> even if there are no Vars in the expression.  This would make it
> act much more like the regular-query context that
> find_expr_references_walker is expecting --- in essence, since
> we're fabricating a single-element rtable for
> find_expr_references_walker to work with, we should fabricate the
> implied whole-table dependency entry too.  But that seems a bit
> weird too, and in particular it's not obvious whether to do that
> if in fact the expression is empty, or doesn't contain any Var at
> all.

This one seems sensible *if* you assume that by the time it is
called there is a known dependency on the particular relation -- for
example, you are dealing with an index on that relation.  Is that a
reasonable restriction on the use of the
recordDependencyOnSingleRelExpr function?  If this was done, would
it allow simplification of the index_create code you showed in #1?

-Kevin

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 3. Or, perhaps we could change recordDependencyOnSingleRelExpr so
>> that it generates a whole-table dependency on the target relation
>> even if there are no Vars in the expression.  This would make it
>> act much more like the regular-query context that
>> find_expr_references_walker is expecting --- in essence, since
>> we're fabricating a single-element rtable for
>> find_expr_references_walker to work with, we should fabricate the
>> implied whole-table dependency entry too.  But that seems a bit
>> weird too, and in particular it's not obvious whether to do that
>> if in fact the expression is empty, or doesn't contain any Var at
>> all.

> This one seems sensible *if* you assume that by the time it is
> called there is a known dependency on the particular relation -- for
> example, you are dealing with an index on that relation.  Is that a
> reasonable restriction on the use of the
> recordDependencyOnSingleRelExpr function?  If this was done, would
> it allow simplification of the index_create code you showed in #1?

Well, it doesn't really improve matters for the index_create code.
What it basically accomplishes so far as that's concerned is to
guarantee that there will be a (likely redundant) whole-table
dependency; which we could equally well guarantee from the other end,
a la my fix #1.

After sleeping on it I'm pretty well convinced that fix #1 is the
way to go; it's simple and gets rid of some code that was just trying
to be too cute.  If we had a clear example of some future use of
recordDependencyOnSingleRelExpr that would require a different behavior
for the expression-dependency-extraction code, I might want to change
that code instead; but we don't.

(Obviously some more comments around the dependency-extraction code will
be a good idea in any case.)

            regards, tom lane