Modify find_inheritance_children() and find_all_inheritors() to add the
authorTom Lane
Tue, 12 May 2009 03:11:02 +0000 (03:11 +0000)
committerTom Lane
Tue, 12 May 2009 03:11:02 +0000 (03:11 +0000)
ability to lock relations as they scan pg_inherits, and to ignore any
relations that have disappeared by the time we get lock on them.  This
makes uses of these functions safe against concurrent DROP operations
on child tables: we will effectively ignore any just-dropped child,
rather than possibly throwing an error as in recent bug report from
Thomas Johansson (and similar past complaints).  The behavior should
not change otherwise, since the code was acquiring those same locks
anyway, just a little bit later.

An exception is LockTableCommand(), which is still behaving unsafely;
but that seems to require some more discussion before we change it.

src/backend/catalog/pg_inherits.c
src/backend/commands/lockcmds.c
src/backend/commands/tablecmds.c
src/backend/optimizer/prep/prepunion.c
src/backend/parser/parse_coerce.c
src/include/catalog/pg_inherits.h
src/include/catalog/pg_inherits_fn.h [new file with mode: 0644]

index c5f035aafe6b4f9a148944777b73c76ef77b983b..49342dc3453ad1570843baa2d1966966f6e1ccf4 100644 (file)
@@ -13,7 +13,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/catalog/pg_inherits.c,v 1.1 2009/05/12 00:56:05 tgl Exp $
+ *   $PostgreSQL: pgsql/src/backend/catalog/pg_inherits.c,v 1.2 2009/05/12 03:11:01 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -22,7 +22,9 @@
 #include "access/heapam.h"
 #include "catalog/pg_class.h"
 #include "catalog/pg_inherits.h"
+#include "catalog/pg_inherits_fn.h"
 #include "parser/parse_type.h"
+#include "storage/lmgr.h"
 #include "utils/fmgroids.h"
 #include "utils/syscache.h"
 #include "utils/tqual.h"
  *
  * Returns a list containing the OIDs of all relations which
  * inherit *directly* from the relation with OID 'parentrelId'.
+ *
+ * The specified lock type is acquired on each child relation (but not on the
+ * given rel; caller should already have locked it).  If lockmode is NoLock
+ * then no locks are acquired, but caller must beware of race conditions
+ * against possible DROPs of child relations.
  */
 List *
-find_inheritance_children(Oid parentrelId)
+find_inheritance_children(Oid parentrelId, LOCKMODE lockmode)
 {
    List       *list = NIL;
    Relation    relation;
@@ -63,13 +70,38 @@ find_inheritance_children(Oid parentrelId)
                BTEqualStrategyNumber, F_OIDEQ,
                ObjectIdGetDatum(parentrelId));
    scan = heap_beginscan(relation, SnapshotNow, 1, key);
+
    while ((inheritsTuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
    {
        inhrelid = ((Form_pg_inherits) GETSTRUCT(inheritsTuple))->inhrelid;
+
+       if (lockmode != NoLock)
+       {
+           /* Get the lock to synchronize against concurrent drop */
+           LockRelationOid(inhrelid, lockmode);
+
+           /*
+            * Now that we have the lock, double-check to see if the relation
+            * really exists or not.  If not, assume it was dropped while
+            * we waited to acquire lock, and ignore it.
+            */
+           if (!SearchSysCacheExists(RELOID,
+                                     ObjectIdGetDatum(inhrelid),
+                                     0, 0, 0))
+           {
+               /* Release useless lock */
+               UnlockRelationOid(inhrelid, lockmode);
+               /* And ignore this relation */
+               continue;
+           }
+       }
+
        list = lappend_oid(list, inhrelid);
    }
+
    heap_endscan(scan);
    heap_close(relation, AccessShareLock);
+
    return list;
 }
 
@@ -78,9 +110,14 @@ find_inheritance_children(Oid parentrelId)
  * find_all_inheritors -
  *     Returns a list of relation OIDs including the given rel plus
  *     all relations that inherit from it, directly or indirectly.
+ *
+ * The specified lock type is acquired on all child relations (but not on the
+ * given rel; caller should already have locked it).  If lockmode is NoLock
+ * then no locks are acquired, but caller must beware of race conditions
+ * against possible DROPs of child relations.
  */
 List *
-find_all_inheritors(Oid parentrelId)
+find_all_inheritors(Oid parentrelId, LOCKMODE lockmode)
 {
    List       *rels_list;
    ListCell   *l;
@@ -100,7 +137,7 @@ find_all_inheritors(Oid parentrelId)
        List       *currentchildren;
 
        /* Get the direct children of this rel */
-       currentchildren = find_inheritance_children(currentrel);
+       currentchildren = find_inheritance_children(currentrel, lockmode);
 
        /*
         * Add to the queue only those children not already seen. This avoids
index 3b31db76fa216da205caf6dc84758ed6a479e8a7..84b152a20075851e212b61b7608bcc13d7db6d5b 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/commands/lockcmds.c,v 1.22 2009/05/12 00:56:05 tgl Exp $
+ *   $PostgreSQL: pgsql/src/backend/commands/lockcmds.c,v 1.23 2009/05/12 03:11:01 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -16,7 +16,7 @@
 
 #include "access/heapam.h"
 #include "catalog/namespace.h"
-#include "catalog/pg_inherits.h"
+#include "catalog/pg_inherits_fn.h"
 #include "commands/lockcmds.h"
 #include "miscadmin.h"
 #include "parser/parse_clause.h"
@@ -48,8 +48,9 @@ LockTableCommand(LockStmt *lockstmt)
 
        reloid = RangeVarGetRelid(relation, false);
 
+       /* XXX NoLock here is not really a good idea */
        if (recurse)
-           children_and_self = find_all_inheritors(reloid);
+           children_and_self = find_all_inheritors(reloid, NoLock);
        else
            children_and_self = list_make1_oid(reloid);
 
index 76e5cf6596e1e80e5db49030099832851ae32429..c9a5dc2a7442b45ff557e79f561cd9e8c0badc75 100644 (file)
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.283 2009/05/12 00:56:05 tgl Exp $
+ *   $PostgreSQL: pgsql/src/backend/commands/tablecmds.c,v 1.284 2009/05/12 03:11:01 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -29,6 +29,7 @@
 #include "catalog/pg_constraint.h"
 #include "catalog/pg_depend.h"
 #include "catalog/pg_inherits.h"
+#include "catalog/pg_inherits_fn.h"
 #include "catalog/pg_namespace.h"
 #include "catalog/pg_opclass.h"
 #include "catalog/pg_tablespace.h"
@@ -796,7 +797,7 @@ ExecuteTruncate(TruncateStmt *stmt)
            ListCell   *child;
            List       *children;
 
-           children = find_all_inheritors(myrelid);
+           children = find_all_inheritors(myrelid, AccessExclusiveLock);
 
            foreach(child, children)
            {
@@ -805,7 +806,8 @@ ExecuteTruncate(TruncateStmt *stmt)
                if (list_member_oid(relids, childrelid))
                    continue;
 
-               rel = heap_open(childrelid, AccessExclusiveLock);
+               /* find_all_inheritors already got lock */
+               rel = heap_open(childrelid, NoLock);
                truncate_check_rel(rel);
                rels = lappend(rels, rel);
                relids = lappend_oid(relids, childrelid);
@@ -1871,7 +1873,7 @@ renameatt(Oid myrelid,
        ListCell   *child;
        List       *children;
 
-       children = find_all_inheritors(myrelid);
+       children = find_all_inheritors(myrelid, AccessExclusiveLock);
 
        /*
         * find_all_inheritors does the recursive search of the inheritance
@@ -1895,7 +1897,7 @@ renameatt(Oid myrelid,
         * tables; else the rename would put them out of step.
         */
        if (!recursing &&
-           find_inheritance_children(myrelid) != NIL)
+           find_inheritance_children(myrelid, NoLock) != NIL)
            ereport(ERROR,
                    (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
                     errmsg("inherited column \"%s\" must be renamed in child tables too",
@@ -3289,7 +3291,7 @@ ATSimpleRecursion(List **wqueue, Relation rel,
        ListCell   *child;
        List       *children;
 
-       children = find_all_inheritors(relid);
+       children = find_all_inheritors(relid, AccessExclusiveLock);
 
        /*
         * find_all_inheritors does the recursive search of the inheritance
@@ -3303,7 +3305,8 @@ ATSimpleRecursion(List **wqueue, Relation rel,
 
            if (childrelid == relid)
                continue;
-           childrel = relation_open(childrelid, AccessExclusiveLock);
+           /* find_all_inheritors already got lock */
+           childrel = relation_open(childrelid, NoLock);
            CheckTableNotInUse(childrel, "ALTER TABLE");
            ATPrepCmd(wqueue, childrel, cmd, false, true);
            relation_close(childrel, NoLock);
@@ -3327,14 +3330,15 @@ ATOneLevelRecursion(List **wqueue, Relation rel,
    ListCell   *child;
    List       *children;
 
-   children = find_inheritance_children(relid);
+   children = find_inheritance_children(relid, AccessExclusiveLock);
 
    foreach(child, children)
    {
        Oid         childrelid = lfirst_oid(child);
        Relation    childrel;
 
-       childrel = relation_open(childrelid, AccessExclusiveLock);
+       /* find_inheritance_children already got lock */
+       childrel = relation_open(childrelid, NoLock);
        CheckTableNotInUse(childrel, "ALTER TABLE");
        ATPrepCmd(wqueue, childrel, cmd, true, true);
        relation_close(childrel, NoLock);
@@ -3480,7 +3484,7 @@ ATPrepAddColumn(List **wqueue, Relation rel, bool recurse,
         * If we are told not to recurse, there had better not be any child
         * tables; else the addition would put them out of step.
         */
-       if (find_inheritance_children(RelationGetRelid(rel)) != NIL)
+       if (find_inheritance_children(RelationGetRelid(rel), NoLock) != NIL)
            ereport(ERROR,
                    (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
                     errmsg("column must be added to child tables too")));
@@ -4199,7 +4203,8 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
     * 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.
     */
-   children = find_inheritance_children(RelationGetRelid(rel));
+   children = find_inheritance_children(RelationGetRelid(rel),
+                                        AccessExclusiveLock);
 
    if (children)
    {
@@ -4213,7 +4218,8 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
            Relation    childrel;
            Form_pg_attribute childatt;
 
-           childrel = heap_open(childrelid, AccessExclusiveLock);
+           /* find_inheritance_children already got lock */
+           childrel = heap_open(childrelid, NoLock);
            CheckTableNotInUse(childrel, "ALTER TABLE");
 
            tuple = SearchSysCacheCopyAttName(childrelid, colName);
@@ -4509,7 +4515,8 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
     * 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.
     */
-   children = find_inheritance_children(RelationGetRelid(rel));
+   children = find_inheritance_children(RelationGetRelid(rel),
+                                        AccessExclusiveLock);
 
    /*
     * If we are told not to recurse, there had better not be any child
@@ -4526,7 +4533,8 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
        Relation    childrel;
        AlteredTableInfo *childtab;
 
-       childrel = heap_open(childrelid, AccessExclusiveLock);
+       /* find_inheritance_children already got lock */
+       childrel = heap_open(childrelid, NoLock);
        CheckTableNotInUse(childrel, "ALTER TABLE");
 
        /* Find or create work queue entry for this table */
@@ -5426,7 +5434,8 @@ ATExecDropConstraint(Relation rel, const char *constrName,
     * use find_all_inheritors to do it in one pass.
     */
    if (is_check_constraint)
-       children = find_inheritance_children(RelationGetRelid(rel));
+       children = find_inheritance_children(RelationGetRelid(rel),
+                                            AccessExclusiveLock);
    else
        children = NIL;
 
@@ -5435,7 +5444,8 @@ ATExecDropConstraint(Relation rel, const char *constrName,
        Oid         childrelid = lfirst_oid(child);
        Relation    childrel;
 
-       childrel = heap_open(childrelid, AccessExclusiveLock);
+       /* find_inheritance_children already got lock */
+       childrel = heap_open(childrelid, NoLock);
        CheckTableNotInUse(childrel, "ALTER TABLE");
 
        ScanKeyInit(&key,
@@ -5659,7 +5669,7 @@ ATPrepAlterColumnType(List **wqueue,
    if (recurse)
        ATSimpleRecursion(wqueue, rel, cmd, recurse);
    else if (!recursing &&
-            find_inheritance_children(RelationGetRelid(rel)) != NIL)
+            find_inheritance_children(RelationGetRelid(rel), NoLock) != NIL)
        ereport(ERROR,
                (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
                 errmsg("type of inherited column \"%s\" must be changed in child tables too",
@@ -6945,8 +6955,11 @@ ATExecAddInherit(Relation child_rel, RangeVar *parent)
     * exclusive locks on the entire inheritance tree, which is a cure worse
     * than the disease.  find_all_inheritors() will cope with circularity
     * anyway, so don't sweat it too much.
+    *
+    * We use weakest lock we can on child's children, namely AccessShareLock.
     */
-   children = find_all_inheritors(RelationGetRelid(child_rel));
+   children = find_all_inheritors(RelationGetRelid(child_rel),
+                                  AccessShareLock);
 
    if (list_member_oid(children, RelationGetRelid(parent_rel)))
        ereport(ERROR,
index a067ed36e69bf83470b4a6f07994581133cf516f..09acdaca65ebcb196a434cb503c28337b6dad566 100644 (file)
@@ -22,7 +22,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/optimizer/prep/prepunion.c,v 1.169 2009/05/12 00:56:05 tgl Exp $
+ *   $PostgreSQL: pgsql/src/backend/optimizer/prep/prepunion.c,v 1.170 2009/05/12 03:11:01 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -32,7 +32,7 @@
 #include "access/heapam.h"
 #include "access/sysattr.h"
 #include "catalog/namespace.h"
-#include "catalog/pg_inherits.h"
+#include "catalog/pg_inherits_fn.h"
 #include "catalog/pg_type.h"
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"
@@ -1157,8 +1157,29 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
        return;
    }
 
-   /* Scan for all members of inheritance set */
-   inhOIDs = find_all_inheritors(parentOID);
+   /*
+    * The rewriter should already have obtained an appropriate lock on each
+    * relation named in the query.  However, for each child relation we add
+    * to the query, we must obtain an appropriate lock, because this will be
+    * the first use of those relations in the parse/rewrite/plan pipeline.
+    *
+    * If the parent relation is the query's result relation, then we need
+    * RowExclusiveLock.  Otherwise, if it's accessed FOR UPDATE/SHARE, we
+    * need RowShareLock; otherwise AccessShareLock.  We can't just grab
+    * AccessShareLock because then the executor would be trying to upgrade
+    * the lock, leading to possible deadlocks.  (This code should match the
+    * parser and rewriter.)
+    */
+   oldrc = get_rowmark(parse, rti);
+   if (rti == parse->resultRelation)
+       lockmode = RowExclusiveLock;
+   else if (oldrc)
+       lockmode = RowShareLock;
+   else
+       lockmode = AccessShareLock;
+
+   /* Scan for all members of inheritance set, acquire needed locks */
+   inhOIDs = find_all_inheritors(parentOID, lockmode);
 
    /*
     * Check that there's at least one descendant, else treat as no-child
@@ -1173,40 +1194,19 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
    }
 
    /*
-    * Find out if parent relation is selected FOR UPDATE/SHARE.  If so,
-    * we need to mark its RowMarkClause as isParent = true, and generate
-    * a new RowMarkClause for each child.
+    * If parent relation is selected FOR UPDATE/SHARE, we need to mark its
+    * RowMarkClause as isParent = true, and generate a new RowMarkClause for
+    * each child.
     */
-   oldrc = get_rowmark(parse, rti);
    if (oldrc)
        oldrc->isParent = true;
 
    /*
     * Must open the parent relation to examine its tupdesc.  We need not lock
-    * it since the rewriter already obtained at least AccessShareLock on each
-    * relation used in the query.
+    * it; we assume the rewriter already did.
     */
    oldrelation = heap_open(parentOID, NoLock);
 
-   /*
-    * However, for each child relation we add to the query, we must obtain an
-    * appropriate lock, because this will be the first use of those relations
-    * in the parse/rewrite/plan pipeline.
-    *
-    * If the parent relation is the query's result relation, then we need
-    * RowExclusiveLock.  Otherwise, if it's accessed FOR UPDATE/SHARE, we
-    * need RowShareLock; otherwise AccessShareLock.  We can't just grab
-    * AccessShareLock because then the executor would be trying to upgrade
-    * the lock, leading to possible deadlocks.  (This code should match the
-    * parser and rewriter.)
-    */
-   if (rti == parse->resultRelation)
-       lockmode = RowExclusiveLock;
-   else if (oldrc)
-       lockmode = RowShareLock;
-   else
-       lockmode = AccessShareLock;
-
    /* Scan the inheritance set and expand it */
    appinfos = NIL;
    foreach(l, inhOIDs)
@@ -1217,9 +1217,9 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
        Index       childRTindex;
        AppendRelInfo *appinfo;
 
-       /* Open rel, acquire the appropriate lock type */
+       /* Open rel if needed; we already have required locks */
        if (childOID != parentOID)
-           newrelation = heap_open(childOID, lockmode);
+           newrelation = heap_open(childOID, NoLock);
        else
            newrelation = oldrelation;
 
index 81f4aa5aa639712f5df301197fe832baddd5ca41..8513741fa45e0a0fb832e28840813ccd0e1215ce 100644 (file)
@@ -8,14 +8,14 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/parser/parse_coerce.c,v 2.175 2009/05/12 00:56:05 tgl Exp $
+ *   $PostgreSQL: pgsql/src/backend/parser/parse_coerce.c,v 2.176 2009/05/12 03:11:02 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
 #include "postgres.h"
 
 #include "catalog/pg_cast.h"
-#include "catalog/pg_inherits.h"
+#include "catalog/pg_inherits_fn.h"
 #include "catalog/pg_proc.h"
 #include "catalog/pg_type.h"
 #include "nodes/makefuncs.h"
index 42c3811ef3ee7a05ed2e7c2602bd89cc6449cd09..9773e007922588378359814fcb23887ad2dd4a1c 100644 (file)
@@ -8,7 +8,7 @@
  * Portions Copyright (c) 1996-2009, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/catalog/pg_inherits.h,v 1.27 2009/05/12 00:56:05 tgl Exp $
+ * $PostgreSQL: pgsql/src/include/catalog/pg_inherits.h,v 1.28 2009/05/12 03:11:02 tgl Exp $
  *
  * NOTES
  *   the genbki.sh script reads this file and generates .bki
@@ -20,7 +20,6 @@
 #define PG_INHERITS_H
 
 #include "catalog/genbki.h"
-#include "nodes/pg_list.h"
 
 /* ----------------
  *     pg_inherits definition.  cpp turns this into
@@ -57,12 +56,4 @@ typedef FormData_pg_inherits *Form_pg_inherits;
  * ----------------
  */
 
-/*
- * prototypes for functions in pg_inherits.c
- */
-extern List *find_inheritance_children(Oid parentrelId);
-extern List *find_all_inheritors(Oid parentrelId);
-extern bool has_subclass(Oid relationId);
-extern bool typeInheritsFrom(Oid subclassTypeId, Oid superclassTypeId);
-
 #endif   /* PG_INHERITS_H */
diff --git a/src/include/catalog/pg_inherits_fn.h b/src/include/catalog/pg_inherits_fn.h
new file mode 100644 (file)
index 0000000..5fb8467
--- /dev/null
@@ -0,0 +1,25 @@
+/*-------------------------------------------------------------------------
+ *
+ * pg_inherits_fn.h
+ *   prototypes for functions in catalog/pg_inherits.c
+ *
+ *
+ * Portions Copyright (c) 1996-2009, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * $PostgreSQL: pgsql/src/include/catalog/pg_inherits_fn.h,v 1.1 2009/05/12 03:11:02 tgl Exp $
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef PG_INHERITS_FN_H
+#define PG_INHERITS_FN_H
+
+#include "nodes/pg_list.h"
+#include "storage/lock.h"
+
+extern List *find_inheritance_children(Oid parentrelId, LOCKMODE lockmode);
+extern List *find_all_inheritors(Oid parentrelId, LOCKMODE lockmode);
+extern bool has_subclass(Oid relationId);
+extern bool typeInheritsFrom(Oid subclassTypeId, Oid superclassTypeId);
+
+#endif   /* PG_INHERITS_FN_H */