Re: index on function confuses drop table cascade on child

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