Re: patch for check constraints using multiple inheritance

Robert Haas wrote:
> On Mon, Aug 2, 2010 at 9:20 AM, Yeb Havinga <yebhavinga@gmail.com> wrote:
>> The attached patch uses the globally defined list.
> I can't speak for any other committer, but personally I'm prepared to
> reject out of hand any solution involving a global variable.  This
> code is none to easy to follow as it is and adding global variables to
> the mix is not going to make it better, even if we can convince
> ourselves that it's safe and correct.  This is something of a corner
> case, so I won't be crushed if a cleaner fix is too invasive to
> back-patch.
Hello Robert,

Thanks for looking at the patch. I've attached a bit more wordy version,
that adds a boolean to AlteredTableInfo and a function to wipe that
boolean between processing of subcommands.
>   Incidentally, we need to shift this discussion to a new
> subject line; we have a working patch for the original subject of this
> thread, and are now discussing the related problem I found with
> attributes.
>
Both solutions have changes in callers of 'find_inheritance_children'. I
was working in the hope a unifiying solution would pop up naturally, but
so far it has not. Checking of the new AlteredTableInfo->relVisited
boolean could perhaps be pushed down into find_inheritance_children, if
multiple-'doing things' for the childs under a multiple-inheriting
relation is unwanted for every kind of action. It seems to me that the
question whether that is the case must be answered, before the current
working patch for coninhcount is 'ready for committer'.

regards,
Yeb Havinga

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

 static List *on_commits = NIL;

-
 /*
  * State information for ALTER TABLE
  *
@@ -132,6 +131,7 @@ typedef struct AlteredTableInfo
     Oid            relid;            /* Relation to work on */
     char        relkind;        /* Its relkind */
     TupleDesc    oldDesc;        /* Pre-modification tuple descriptor */
+    bool        relVisited;     /* Was the rel visited before, for the current subcommand */
     /* Information saved by Phase 1 for Phase 2: */
     List       *subcmds[AT_NUM_PASSES]; /* Lists of AlterTableCmd */
     /* Information saved by Phases 1/2 for Phase 3: */
@@ -335,6 +335,7 @@ static void ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode)
 static void copy_relation_data(SMgrRelation rel, SMgrRelation dst,
                    ForkNumber forkNum, bool istemp);
 static const char *storage_name(char c);
+static void ATResetRelVisited(List **wqueue);


 /* ----------------------------------------------------------------
@@ -2583,6 +2584,7 @@ ATController(Relation rel, List *cmds, bool recurse, LOCKMODE lockmode)
     {
         AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd);

+        ATResetRelVisited(&wqueue);
         ATPrepCmd(&wqueue, rel, cmd, recurse, false, lockmode);
     }

@@ -3503,6 +3505,23 @@ ATGetQueueEntry(List **wqueue, Relation rel)
 }

 /*
+ * ATResetRelVisited: reset the visitation info for each rel in the working
+ * queue, so it can be used for the next subcommand.
+ */
+static void
+ATResetRelVisited(List **wqueue)
+{
+    AlteredTableInfo *tab;
+    ListCell   *ltab;
+
+    foreach(ltab, *wqueue)
+    {
+        tab = (AlteredTableInfo *) lfirst(ltab);
+        tab->relVisited = false;
+    }
+}
+
+/*
  * ATSimplePermissions
  *
  * - Ensure that it is a relation (or possibly a view)
@@ -3618,19 +3637,29 @@ 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,
                     AlterTableCmd *cmd, LOCKMODE lockmode)
 {
     Oid            relid = RelationGetRelid(rel);
+    AlteredTableInfo *tab = ATGetQueueEntry(wqueue, 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 (tab->relVisited)
+        return;
+    tab->relVisited = true;
+
     children = find_inheritance_children(relid, lockmode);

     foreach(child, children)
@@ -4891,6 +4920,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.