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);
extern bool 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 event triggers
@@ -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 --- ownership of
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);
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
