[HACKERS] Race between SELECT and ALTER TABLE NO INHERIT

Hello.

I had a case of unexpected error caused by ALTER TABLE NO
INHERIT.

=# CREATE TABLE p (a int);
=# CREATE TABLE c1 () INHERITS (p);

session A=# BEGIN;
session A=# ALTER TABLE c1 NO INHERIT p;

session B=# EXPLAIN ANALYZE SELECT * FROM p;
(blocked)

session A=# COMMIT;

session B: ERROR:  could not find inherited attribute "a" of relation "c1"

This happens at least back to 9.1 to master and doesn't seem to
be a designed behavior.

The cause is that NO INHERIT doesn't take an exlusive lock on the
parent. This allows expand_inherited_rtentry to add the child
relation into appendrel after removal from the inheritance but
still exists.

I see two ways to fix this.

The first patch adds a recheck of inheritance relationship if the
corresponding attribute is missing in the child in
make_inh_translation_list(). The recheck is a bit complex but it
is not performed unless the sequence above is happen. It checks
duplication of relid (or cycles in inheritance) following
find_all_inheritors (but doing a bit different) but I'm not sure
it is really useful.


The second patch lets ALTER TABLE NO INHERIT to acquire locks on
the parent first.

Since the latter has a larger impact on the current behavior and
we already treat "DROP TABLE child" case in the similar way, I
suppose that the first approach would be preferable.


Any comments or thoughts?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/catalog/pg_inherits.c b/src/backend/catalog/pg_inherits.c
index e5fb52c..1670d11 100644
--- a/src/backend/catalog/pg_inherits.c
+++ b/src/backend/catalog/pg_inherits.c
@@ -42,6 +42,8 @@ typedef struct SeenRelsEntry    ListCell   *numparents_cell;    /* corresponding list cell */}
SeenRelsEntry;
+static bool is_descendent_of_internal(Oid parentId, Oid childId,
+                                      HTAB *seen_rels);/* * find_inheritance_children *
@@ -376,3 +378,71 @@ typeInheritsFrom(Oid subclassTypeId, Oid superclassTypeId)    return result;}
+
+/*
+ * Check if the child is really a descendent of the parent
+ */
+bool
+is_descendent_of(Oid parentId, Oid childId)
+{
+    HTAB       *seen_rels;
+    HASHCTL        ctl;
+    bool        ischild = false;
+
+    memset(&ctl, 0, sizeof(ctl));
+    ctl.keysize = sizeof(Oid);
+    ctl.entrysize = sizeof(Oid);
+    ctl.hcxt = CurrentMemoryContext;
+
+    seen_rels = hash_create("is_descendent_of temporary table",
+                            32, /* start small and extend */
+                            &ctl,
+                            HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
+
+    ischild = is_descendent_of_internal(parentId, childId, seen_rels);
+
+    hash_destroy(seen_rels);
+
+    return ischild;
+}
+
+static bool
+is_descendent_of_internal(Oid parentId, Oid childId, HTAB *seen_rels)
+{
+    Relation    inhrel;
+    SysScanDesc scan;
+    ScanKeyData key[1];
+    bool        ischild = false;
+    HeapTuple    inheritsTuple;
+
+    inhrel = heap_open(InheritsRelationId, AccessShareLock);
+    ScanKeyInit(&key[0], Anum_pg_inherits_inhparent,
+                BTEqualStrategyNumber, F_OIDEQ,    ObjectIdGetDatum(parentId));
+    scan = systable_beginscan(inhrel, InheritsParentIndexId, true,
+                              NULL, 1, key);
+
+    while ((inheritsTuple = systable_getnext(scan)) != NULL)
+    {
+        bool found;
+        Oid inhrelid = ((Form_pg_inherits) GETSTRUCT(inheritsTuple))->inhrelid;
+
+        hash_search(seen_rels, &inhrelid, HASH_ENTER, &found);
+
+        /*
+         * Recursively check into children. Although there can't theoretically
+         * be any cycles in the inheritance graph, check the cycles following
+         * find_all_inheritors.
+         */
+        if (inhrelid == childId ||
+            (!found && is_descendent_of_internal(inhrelid, childId, seen_rels)))
+        {
+            ischild = true;
+            break;
+        }
+    }
+
+    systable_endscan(scan);
+    heap_close(inhrel, AccessShareLock);
+
+    return ischild;
+}
diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c
index cf46b74..f1c582a 100644
--- a/src/backend/optimizer/prep/prepunion.c
+++ b/src/backend/optimizer/prep/prepunion.c
@@ -99,10 +99,9 @@ static List *generate_append_tlist(List *colTypes, List *colCollations,static List
*generate_setop_grouplist(SetOperationStmt*op, List *targetlist);static void expand_inherited_rtentry(PlannerInfo
*root,RangeTblEntry *rte,                         Index rti);
 
-static void make_inh_translation_list(Relation oldrelation,
+static List *make_inh_translation_list(Relation oldrelation,                          Relation newrelation,
-                          Index newvarno,
-                          List **translated_vars);
+                          Index newvarno);static Bitmapset *translate_col_privs(const Bitmapset *parent_privs,
          List *translated_vars);static Node *adjust_appendrel_attrs_mutator(Node *node,
 
@@ -1502,14 +1501,28 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)         */        if
(childrte->relkind!= RELKIND_PARTITIONED_TABLE)        {
 
+            List *translated_vars =
+                make_inh_translation_list(oldrelation, newrelation,
+                                          childRTindex);
+
+            if (!translated_vars)
+            {
+                /*
+                 * This childrel is no longer a child of the parent.
+                 * Close the child relation releasing locks.
+                 */
+                if (childOID != parentOID)
+                    heap_close(newrelation, lockmode);
+                continue;
+            }
+            need_append = true;            appinfo = makeNode(AppendRelInfo);            appinfo->parent_relid = rti;
         appinfo->child_relid = childRTindex;            appinfo->parent_reltype = oldrelation->rd_rel->reltype;
   appinfo->child_reltype = newrelation->rd_rel->reltype;
 
-            make_inh_translation_list(oldrelation, newrelation, childRTindex,
-                                      &appinfo->translated_vars);
+            appinfo->translated_vars = translated_vars;            appinfo->parent_reloid = parentOID;
appinfos= lappend(appinfos, appinfo);
 
@@ -1614,14 +1627,14 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)/* *
make_inh_translation_list*      Build the list of translations from parent Vars to child Vars for
 
- *      an inheritance child.
+ *      an inheritance child. Returns NULL if the two relations are no longer in
+ *      a inheritance relationship. * * For paranoia's sake, we match type/collation as well as attribute name. */
-static void
+static List *make_inh_translation_list(Relation oldrelation, Relation newrelation,
-                          Index newvarno,
-                          List **translated_vars)
+                          Index newvarno){    List       *vars = NIL;    TupleDesc    old_tupdesc =
RelationGetDescr(oldrelation);
@@ -1691,8 +1704,18 @@ make_inh_translation_list(Relation oldrelation, Relation newrelation,                    break;
         }            if (new_attno >= newnatts)
 
+            {
+                /*
+                 * It's possible that newrelation is no longer a child of the
+                 * newrelation. We just ingore it for the case.
+                 */
+                if (!is_descendent_of(oldrelation->rd_id, newrelation->rd_id))
+                    return NULL;
+
+                /* If still a child, something's wrong. */                elog(ERROR, "could not find inherited
attribute\"%s\" of relation \"%s\"",                     attname, RelationGetRelationName(newrelation));
 
+            }        }        /* Found it, check type and collation match */
@@ -1711,7 +1734,7 @@ make_inh_translation_list(Relation oldrelation, Relation newrelation,
       0));    }
 
-    *translated_vars = vars;
+    return vars;}/*
diff --git a/src/include/catalog/pg_inherits_fn.h b/src/include/catalog/pg_inherits_fn.h
index abfa476..3affe40 100644
--- a/src/include/catalog/pg_inherits_fn.h
+++ b/src/include/catalog/pg_inherits_fn.h
@@ -22,5 +22,6 @@ extern List *find_all_inheritors(Oid parentrelId, LOCKMODE lockmode,                    List
**parents);externbool has_subclass(Oid relationId);extern bool typeInheritsFrom(Oid subclassTypeId, Oid
superclassTypeId);
+extern bool is_descendent_of(Oid parentId, Oid childId);#endif                            /* PG_INHERITS_FN_H */
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7d9c769..f69631f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -3397,6 +3397,30 @@ AlterTableGetLockLevel(List *cmds)}/*
+ * Some AT commands require to take a lock on the parent prior to the target.
+ */
+void
+AlterTableLockInhParent(List *cmds, LOCKMODE lockmode)
+{
+    ListCell   *lcmd;
+
+    foreach(lcmd, cmds)
+    {
+        AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd);
+
+        switch (cmd->subtype)
+        {        
+        case AT_DropInherit:
+            RangeVarGetRelid((RangeVar *)cmd->def, lockmode, false);
+            break;
+
+        default:
+            break;
+        }
+    }
+}
+
+/* * ATController provides top level control over the phases. * * parsetree is passed in to allow it to be passed to
eventtriggers
 
@@ -11446,12 +11470,8 @@ ATExecDropInherit(Relation rel, RangeVar *parent, LOCKMODE lockmode)
(errcode(ERRCODE_WRONG_OBJECT_TYPE),                errmsg("cannot change inheritance of a partition")));
 
-    /*
-     * AccessShareLock on the parent is probably enough, seeing that DROP
-     * TABLE doesn't lock parent tables at all.  We need some lock since we'll
-     * be inspecting the parent's schema.
-     */
-    parent_rel = heap_openrv(parent, AccessShareLock);
+    /* Requreid lock is already held */
+    parent_rel = heap_openrv(parent, NoLock);    /*     * We don't bother to check ownership of the parent table ---
ownershipof
 
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index ddacac8..ce3406b 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1102,6 +1102,9 @@ ProcessUtilitySlow(ParseState *pstate,                     * permissions.                     */
                 lockmode = AlterTableGetLockLevel(atstmt->cmds);
 
+
+                    /* Lock the parent first if required */
+                    AlterTableLockInhParent(atstmt->cmds, lockmode);                    relid =
AlterTableLookupRelation(atstmt,lockmode);                    if (OidIsValid(relid))
 
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index abd31b6..c643326 100644
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -27,6 +27,7 @@ extern ObjectAddress DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,extern void
RemoveRelations(DropStmt*drop);
 
+void AlterTableLockInhParent(List *cmds, LOCKMODE lockmode);extern Oid    AlterTableLookupRelation(AlterTableStmt
*stmt,LOCKMODE lockmode);extern void AlterTable(Oid relid, LOCKMODE lockmode, AlterTableStmt *stmt);