Robert Haas wrote:
On Mon, Aug 2, 2010 at 9:20 AM, Yeb Havinga <yebhavi...@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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
