Bug with updateable Views and inherited tables?

[Sebastian Böck]

Hello all,

i have a view defined as a simple select of a table.
This table is inherited by a couple of others.
All entries belong to the child-tables.

I also have an unconditional update rule on the view.
If i do an update to the view, the update is rewritten
to update the father-table.

And now a strange thing is happening:
If i do an update, this update is done correctly only
on the first defined child-table. If it applies to
one of the other 8 child-tables nothing happens at all.

If i update the father-table directly everything is
working like expected (the update is "directed" to the
right table).

Is this a known limitation with views, rules and
inherited tables i haven't heard of? Or is it a bug?

My Postgres is version 7.4.5.

Thanks in advance

Sebastian
---------------------------(end of broadcast)---------------------------
TIP 6: Have you searched our list archives?

http://archives.postgresql.org

[Tom Lane]

=?ISO-8859-1?Q?Sebastian_B=F6ck?= <se************@freenet.de> writes:

Is this a known limitation with views, rules and
inherited tables i haven't heard of? Or is it a bug?

When you haven't shown us any details, it's impossible to tell.
Let's see the actual table, view, and rule definitions. (pg_dump -s
output would be good.)

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 6: Have you searched our list archives?

http://archives.postgresql.org

[Sebastian Böck]

Tom Lane wrote:

=?ISO-8859-1?Q?Sebastian_B=F6ck?= <se************@freenet.de> writes:

Is this a known limitation with views, rules and
inherited tables i haven't heard of? Or is it a bug?

When you haven't shown us any details, it's impossible to tell.
Let's see the actual table, view, and rule definitions. (pg_dump -s
output would be good.)

Since my first attempt to send this message seems to be caught
by spamfilters, i'll repost it.

Sorry for the delay.
I investigated a little bit further and can be more precisely
about the whole thing. This (wrong) behaviour only occurs, if
the view has an order by clause.

To repoduce, simply run the attached script.

Sebastian
CREATE TABLE id (
id SERIAL PRIMARY KEY,
name TEXT
);

CREATE TABLE test_1 (
id INTEGER PRIMARY KEY,
name TEXT
) INHERITS (id);

CREATE TABLE test_2 (
id INTEGER PRIMARY KEY,
name TEXT
) INHERITS (id);

CREATE TABLE test_3 (
id INTEGER PRIMARY KEY,
name TEXT
) INHERITS (id);

CREATE VIEW working AS SELECT * FROM id;
CREATE RULE update_working AS ON UPDATE TO working
DO INSTEAD UPDATE id SET name = NEW.name WHERE id = OLD.id;

CREATE VIEW not_working AS SELECT * FROM id ORDER BY id;
CREATE RULE update_not_working AS ON UPDATE TO not_working
DO INSTEAD UPDATE id SET name = NEW.name WHERE id = OLD.id;

INSERT INTO test_1 (name) VALUES ('Test 1');
INSERT INTO test_1 (name) VALUES ('Test 2');
INSERT INTO test_2 (name) VALUES ('Test 3');
INSERT INTO test_2 (name) VALUES ('Test 4');
INSERT INTO test_3 (name) VALUES ('Test 5');
INSERT INTO test_3 (name) VALUES ('Test 6');

SELECT * FROM working;
UPDATE working SET name = 'working' WHERE id = '1';
UPDATE working SET name = 'working' WHERE id = '3';
UPDATE working SET name = 'working' WHERE id = '5';
SELECT * FROM working;
SELECT * FROM not_working;
UPDATE not_working SET name = 'should work' WHERE id = '2';
UPDATE not_working SET name = 'should work' WHERE id = '4';
UPDATE not_working SET name = 'should work' WHERE id = '5';
SELECT * FROM not_working;
---------------------------(end of broadcast)---------------------------
TIP 7: don't forget to increase your free space map settings

[Tom Lane]

=?ISO-8859-1?Q?Sebastian_B=F6ck?= <se************@freenet.de> writes:

I investigated a little bit further and can be more precisely
about the whole thing. This (wrong) behaviour only occurs, if
the view has an order by clause.

The bug is triggered by the combination of an inherited UPDATE target
and an unflattenable sub-Query. I verified that it's been broken for
as long as we've had such features :-(.

I've applied the attached patch to 8.0. You could probably adapt it for
7.4, but I'm hesitant to put such a nontrivial change into a stable
branch without a lot more testing.

regards, tom lane

*** src/backend/optimizer/path/allpaths.c.orig Sun Aug 29 09:55:03 2004
--- src/backend/optimizer/path/allpaths.c Sat Oct 2 17:23:54 2004
***************
*** 124,131 ****
/* RangeFunction --- generate a separate plan for it */
set_function_pathlist(root, rel, rte);
}
! else if ((inheritlist = expand_inherited_rtentry(root, rti, true))
! != NIL)
{
/* Relation is root of an inheritance tree, process specially */
set_inherited_rel_pathlist(root, rel, rti, rte, inheritlist);
--- 124,130 ----
/* RangeFunction --- generate a separate plan for it */
set_function_pathlist(root, rel, rte);
}
! else if ((inheritlist = expand_inherited_rtentry(root, rti)) != NIL)
{
/* Relation is root of an inheritance tree, process specially */
set_inherited_rel_pathlist(root, rel, rti, rte, inheritlist);
***************
*** 222,234 ****
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("SELECT FOR UPDATE is not supported for inheritance queries")));
-
- /*
- * The executor will check the parent table's access permissions when
- * it examines the parent's inheritlist entry. There's no need to
- * check twice, so turn off access check bits in the original RTE.
- */
- rte->requiredPerms = 0;

/*
* Initialize to compute size estimates for whole inheritance tree
--- 221,226 ----
*** src/backend/optimizer/plan/planner.c.orig Sun Aug 29 22:57:31 2004
--- src/backend/optimizer/plan/planner.c Sat Oct 2 17:57:29 2004
***************
*** 320,327 ****
* grouping_planner.
*/
if (parse->resultRelation &&
! (lst = expand_inherited_rtentry(parse, parse->resultRelation,
! false)) != NIL)
plan = inheritance_planner(parse, lst);
else
plan = grouping_planner(parse, tuple_fraction);
--- 320,326 ----
* grouping_planner.
*/
if (parse->resultRelation &&
! (lst = expand_inherited_rtentry(parse, parse->resultRelation)) != NIL)
plan = inheritance_planner(parse, lst);
else
plan = grouping_planner(parse, tuple_fraction);
***************
*** 513,519 ****
{
int childRTindex = lfirst_int(l);
Oid childOID = getrelid(childRTindex, parse->rtable);
- int subrtlength;
Query *subquery;
Plan *subplan;

--- 512,517 ----
***************
*** 526,547 ****
subplans = lappend(subplans, subplan);

/*
! * It's possible that additional RTEs got added to the rangetable
! * due to expansion of inherited source tables (see allpaths.c).
! * If so, we must copy 'em back to the main parse tree's rtable.
*
! * XXX my goodness this is ugly. Really need to think about ways to
! * rein in planner's habit of scribbling on its input.
*/
! subrtlength = list_length(subquery->rtable);
! if (subrtlength > mainrtlength)
{
! List *subrt;

! subrt = list_copy_tail(subquery->rtable, mainrtlength);
! parse->rtable = list_concat(parse->rtable, subrt);
! mainrtlength = subrtlength;
}
/* Save preprocessed tlist from first rel for use in Append */
if (tlist == NIL)
tlist = subplan->targetlist;
--- 524,563 ----
subplans = lappend(subplans, subplan);

/*
! * XXX my goodness this next bit is ugly. Really need to think about
! * ways to rein in planner's habit of scribbling on its input.
*
! * Planning of the subquery might have modified the rangetable,
! * either by addition of RTEs due to expansion of inherited source
! * tables, or by changes of the Query structures inside subquery
! * RTEs. We have to ensure that this gets propagated back to the
! * master copy. However, if we aren't done planning yet, we also
! * need to ensure that subsequent calls to grouping_planner have
! * virgin sub-Queries to work from. So, if we are at the last
! * list entry, just copy the subquery rangetable back to the master
! * copy; if we are not, then extend the master copy by adding
! * whatever the subquery added. (We assume these added entries
! * will go untouched by the future grouping_planner calls. We are
! * also effectively assuming that sub-Queries will get planned
! * identically each time, or at least that the impacts on their
! * rangetables will be the same each time. Did I say this is ugly?)
*/
! if (lnext(l) == NULL)
! parse->rtable = subquery->rtable;
! else
{
! int subrtlength = list_length(subquery->rtable);

! if (subrtlength > mainrtlength)
! {
! List *subrt;
!
! subrt = list_copy_tail(subquery->rtable, mainrtlength);
! parse->rtable = list_concat(parse->rtable, subrt);
! mainrtlength = subrtlength;
! }
}
+
/* Save preprocessed tlist from first rel for use in Append */
if (tlist == NIL)
tlist = subplan->targetlist;
*** src/backend/optimizer/prep/prepunion.c.orig Sun Aug 29 09:55:05 2004
--- src/backend/optimizer/prep/prepunion.c Sat Oct 2 17:24:22 2004
***************
*** 705,728 ****
* whole inheritance set (parent and children).
* If not, return NIL.
*
! * When dup_parent is false, the initially given RT index is part of the
! * returned list (if any). When dup_parent is true, the given RT index
! * is *not* in the returned list; a duplicate RTE will be made for the
! * parent table.
*
* A childless table is never considered to be an inheritance set; therefore
* the result will never be a one-element list. It'll be either empty
* or have two or more elements.
*
! * NOTE: after this routine executes, the specified RTE will always have
! * its inh flag cleared, whether or not there were any children. This
! * ensures we won't expand the same RTE twice, which would otherwise occur
! * for the case of an inherited UPDATE/DELETE target relation.
! *
! * XXX probably should convert the result type to Relids?
*/
List *
! expand_inherited_rtentry(Query *parse, Index rti, bool dup_parent)
{
RangeTblEntry *rte = rt_fetch(rti, parse->rtable);
Oid parentOID;
--- 705,727 ----
* whole inheritance set (parent and children).
* If not, return NIL.
*
! * Note that the original RTE is considered to represent the whole
! * inheritance set. The first member of the returned list is an RTE
! * for the same table, but with inh = false, to represent the parent table
! * in its role as a simple member of the set. The original RT index is
! * never a member of the returned list.
*
* A childless table is never considered to be an inheritance set; therefore
* the result will never be a one-element list. It'll be either empty
* or have two or more elements.
*
! * Note: there are cases in which this routine will be invoked multiple
! * times on the same RTE. We will generate a separate set of child RTEs
! * for each invocation. This is somewhat wasteful but seems not worth
! * trying to avoid.
*/
List *
! expand_inherited_rtentry(Query *parse, Index rti)
{
RangeTblEntry *rte = rt_fetch(rti, parse->rtable);
Oid parentOID;
***************
*** 734,745 ****
if (!rte->inh)
return NIL;
Assert(rte->rtekind == RTE_RELATION);
- /* Always clear the parent's inh flag, see above comments */
- rte->inh = false;
/* Fast path for common case of childless table */
parentOID = rte->relid;
if (!has_subclass(parentOID))
return NIL;
/* Scan for all members of inheritance set */
inhOIDs = find_all_inheritors(parentOID);

--- 733,746 ----
if (!rte->inh)
return NIL;
Assert(rte->rtekind == RTE_RELATION);
/* Fast path for common case of childless table */
parentOID = rte->relid;
if (!has_subclass(parentOID))
+ {
+ /* Clear flag to save repeated tests if called again */
+ rte->inh = false;
return NIL;
+ }
/* Scan for all members of inheritance set */
inhOIDs = find_all_inheritors(parentOID);

***************
*** 749,784 ****
* table once had a child but no longer does.
*/
if (list_length(inhOIDs) < 2)
return NIL;
! /* OK, it's an inheritance set; expand it */
! if (dup_parent)
! inhRTIs = NIL;
! else
! inhRTIs = list_make1_int(rti); /* include original RTE in result */

foreach(l, inhOIDs)
{
Oid childOID = lfirst_oid(l);
RangeTblEntry *childrte;
Index childRTindex;

- /* parent will be in the list too; skip it if not dup requested */
- if (childOID == parentOID && !dup_parent)
- continue;
-
/*
* Build an RTE for the child, and attach to query's rangetable
* list. We copy most fields of the parent's RTE, but replace
! * relation real name and OID. Note that inh will be false at
! * this point.
*/
childrte = copyObject(rte);
childrte->relid = childOID;
parse->rtable = lappend(parse->rtable, childrte);
childRTindex = list_length(parse->rtable);
-
inhRTIs = lappend_int(inhRTIs, childRTindex);
}

return inhRTIs;
}
--- 750,790 ----
* table once had a child but no longer does.
*/
if (list_length(inhOIDs) < 2)
+ {
+ /* Clear flag to save repeated tests if called again */
+ rte->inh = false;
return NIL;
! }

+ /* OK, it's an inheritance set; expand it */
+ inhRTIs = NIL;
foreach(l, inhOIDs)
{
Oid childOID = lfirst_oid(l);
RangeTblEntry *childrte;
Index childRTindex;

/*
* Build an RTE for the child, and attach to query's rangetable
* list. We copy most fields of the parent's RTE, but replace
! * relation OID, and set inh = false.
*/
childrte = copyObject(rte);
childrte->relid = childOID;
+ childrte->inh = false;
parse->rtable = lappend(parse->rtable, childrte);
childRTindex = list_length(parse->rtable);
inhRTIs = lappend_int(inhRTIs, childRTindex);
}
+
+ /*
+ * The executor will check the parent table's access permissions when
+ * it examines the parent's inheritlist entry. There's no need to
+ * check twice, so turn off access check bits in the original RTE.
+ * (If we are invoked more than once, extra copies of the child RTEs
+ * will also not cause duplicate permission checks.)
+ */
+ rte->requiredPerms = 0;

return inhRTIs;
}
*** src/backend/optimizer/util/clauses.c.orig Sun Aug 29 09:55:06 2004
--- src/backend/optimizer/util/clauses.c Sat Oct 2 15:41:30 2004
***************
*** 3254,3263 ****
--- 3254,3273 ----
CHECKFLATCOPY(newrte->subquery, rte->subquery, Query);
MUTATE(newrte->subquery, newrte->subquery, Query *);
}
+ else
+ {
+ /* else, copy RT subqueries as-is */
+ newrte->subquery = copyObject(rte->subquery);
+ }
break;
case RTE_JOIN:
if (!(flags & QTW_IGNORE_JOINALIASES))
MUTATE(newrte->joinaliasvars, rte->joinaliasvars, List *);
+ else
+ {
+ /* else, copy join aliases as-is */
+ newrte->joinaliasvars = copyObject(rte->joinaliasvars);
+ }
break;
case RTE_FUNCTION:
MUTATE(newrte->funcexpr, rte->funcexpr, Node *);
*** src/include/optimizer/prep.h.orig Sun Aug 29 09:56:05 2004
--- src/include/optimizer/prep.h Sat Oct 2 17:23:45 2004
***************
*** 52,59 ****

extern List *find_all_inheritors(Oid parentrel);

! extern List *expand_inherited_rtentry(Query *parse, Index rti,
! bool dup_parent);

extern Node *adjust_inherited_attrs(Node *node,
Index old_rt_index, Oid old_relid,
--- 52,58 ----

extern List *find_all_inheritors(Oid parentrel);

! extern List *expand_inherited_rtentry(Query *parse, Index rti);

extern Node *adjust_inherited_attrs(Node *node,
Index old_rt_index, Oid old_relid,

---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faqs/FAQ.html

[Sebastian Böck]

Tom Lane wrote:

=?ISO-8859-1?Q?Sebastian_B=F6ck?= <se************@freenet.de> writes:

I investigated a little bit further and can be more precisely
about the whole thing. This (wrong) behaviour only occurs, if
the view has an order by clause.

The bug is triggered by the combination of an inherited UPDATE target
and an unflattenable sub-Query. I verified that it's been broken for
as long as we've had such features :-(.

I've applied the attached patch to 8.0.

Thank you!
You could probably adapt it for
7.4, but I'm hesitant to put such a nontrivial change into a stable
branch without a lot more testing.

It isn't that necessary for me, just wondered 'bout the
strange behaviour.

Sebastian

---------------------------(end of broadcast)---------------------------
TIP 4: Don't 'kill -9' the postmaster