Re: patch for check constraints using multiple inheritance

Robert Haas wrote:
> I agree that's the crux of the problem, but I can't see solving it
> with a global variable.  I realize you were just testing...
>
Yes it was just a test. However, somewhere information must be kept or
altered so it can be detected that a relation has already been visited,
i.e. it is a multiple inheriting child. The other solutions I could
think of are more intrusive (i.e. definitionin ATController and passing
as parameter).

The attached patch uses the globally defined list. After ATPrepCmd the
list pointer is reset to NIL, the list not freed since the allocs are
done in a memory context soon to be deleted (PortalHeapMemory). It
passes regression as well as the script below.

regards,
Yeb Havinga


DROP SCHEMA IF EXISTS test_inheritance CASCADE;
CREATE SCHEMA test_inheritance;
SET search_path TO test_inheritance;

CREATE TABLE top (i int);
CREATE TABLE mid1 () INHERITS (top);
CREATE TABLE mid2 () INHERITS (top);
CREATE TABLE bottom () INHERITS (mid1, mid2);
CREATE TABLE basement () INHERITS (bottom);

ALTER TABLE top
ADD COLUMN a_table_column integer,
ADD COLUMN a_table_column2 integer;

ALTER TABLE top
ADD COLUMN a_table_column3 integer;

ALTER TABLE top
ADD CONSTRAINT  a_check_constraint CHECK (i IN (0,1)),
ADD CONSTRAINT  a_check_constraint2 CHECK (i IN (0,1));

ALTER TABLE top
ADD CONSTRAINT  a_check_constraint3 CHECK (i IN (0,1));

SELECT t.oid, t.relname, a.attinhcount
FROM pg_class t
JOIN pg_attribute a ON (a.attrelid = t.oid)
JOIN pg_namespace n ON (t.relnamespace = n.oid)
WHERE n.nspname = 'test_inheritance' AND a.attname LIKE 'a_table_column%'
ORDER BY oid;

SELECT t.oid, t.relname, c.coninhcount
FROM pg_class t
JOIN pg_constraint c ON (c.conrelid = t.oid)
JOIN pg_namespace n ON (t.relnamespace = n.oid)
WHERE n.nspname = 'test_inheritance'
ORDER BY oid;




diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 49a6f73..08efffc 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -99,6 +99,10 @@ typedef struct OnCommitItem

 static List *on_commits = NIL;

+/*
+ * Per subcommand history of relids visited in an inheritance hierarchy.
+ */
+static List *visited_relids = NIL;

 /*
  * State information for ALTER TABLE
@@ -2584,6 +2588,7 @@ ATController(Relation rel, List *cmds, bool recurse, LOCKMODE lockmode)
         AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd);

         ATPrepCmd(&wqueue, rel, cmd, recurse, false, lockmode);
+        visited_relids = NIL;
     }

     /* Close the relation, but keep lock until commit */
@@ -3618,10 +3623,11 @@ ATSimpleRecursion(List **wqueue, Relation rel,
 /*
  * ATOneLevelRecursion
  *
- * Here, we visit only direct inheritance children.  It is expected that
- * the command's prep routine will recurse again to find indirect children.
- * When using this technique, a multiply-inheriting child will be visited
- * multiple times.
+ * Here, we visit only direct inheritance children.  It is expected that the
+ * command's prep routine will recurse again to find indirect children.  When
+ * using this technique, a multiple-inheriting child will be visited multiple
+ * times. Childs of multiple-inheriting childs however are only visited once
+ * for each parent.
  */
 static void
 ATOneLevelRecursion(List **wqueue, Relation rel,
@@ -3631,6 +3637,14 @@ ATOneLevelRecursion(List **wqueue, Relation rel,
     ListCell   *child;
     List       *children;

+    /* If we already visited the current multiple-inheriting relation, we
+     * mustn't recurse to it's child tables, because they've already been
+     * visited. Visiting them would lead to an incorrect value for
+     * attinhcount. */
+    if (list_member_oid(visited_relids, relid))
+        return;
+    visited_relids = lappend_oid(visited_relids, relid);
+
     children = find_inheritance_children(relid, lockmode);

     foreach(child, children)
@@ -4891,6 +4905,15 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
     CommandCounterIncrement();

     /*
+     * If the constraint got merged with an existing constraint, we're done.
+     * We mustn't recurse to child tables in this case, because they've already
+     * got the constraint, and visiting them again would lead to an incorrect
+     * value for coninhcount.
+     */
+    if (newcons == NIL)
+        return;
+
+    /*
      * Propagate to children as appropriate.  Unlike most other ALTER
      * routines, we have to do this one level of recursion at a time; we can't
      * use find_all_inheritors to do it in one pass.