Обсуждение: 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