Heed lock protocol in DROP OWNED BY

We were acquiring object locks then deleting objects one by one, instead
of acquiring all object locks first, ignoring those that did not exist,
and then deleting all objects together.   The latter is the correct
protocol to use, and what this commits changes to code to do.  Failing
to follow that leads to "cache lookup failed for relation XYZ" error
reports when DROP OWNED runs concurrently with other DDL -- for example,
a session termination that removes some temp tables.

Author: Álvaro Herrera
Reported-by: Mithun Chicklore Yogendra (Mithun CY)
Reviewed-by: Ahsan Hadi, Tom Lane
Discussion: https://postgr.es/m/CADq3xVZTbzK4ZLKq+dn_vB4QafXXbmMgDP3trY-GuLnib2Ai1w@mail.gmail.com

diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 7b00d7c7bab16d88b3bd6d7957a3d08d3d78cbc7..47ad2e83da91aafd22b24523edd1c49892742164 100644 (file)
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -188,8 +188,6 @@ static void reportDependentObjects(const ObjectAddresses *targetObjects,
 static void deleteOneObject(const ObjectAddress *object,
                                Relation *depRel, int32 flags);
 static void doDeletion(const ObjectAddress *object, int flags);
-static void AcquireDeletionLock(const ObjectAddress *object, int flags);
-static void ReleaseDeletionLock(const ObjectAddress *object);
 static bool find_expr_references_walker(Node *node,
                                                        find_expr_references_context *context);
 static void eliminate_duplicate_dependencies(ObjectAddresses *addrs);
@@ -1326,11 +1324,14 @@ doDeletion(const ObjectAddress *object, int flags)
 /*
  * AcquireDeletionLock - acquire a suitable lock for deleting an object
  *
+ * Accepts the same flags as performDeletion (though currently only
+ * PERFORM_DELETION_CONCURRENTLY does anything).
+ *
  * We use LockRelation for relations, LockDatabaseObject for everything
- * else.  Note that dependency.c is not concerned with deleting any kind of
- * shared-across-databases object, so we have no need for LockSharedObject.
+ * else.  Shared-across-databases objects are not currently supported
+ * because no caller cares, but could be modified to use LockSharedObject.
  */
-static void
+void
 AcquireDeletionLock(const ObjectAddress *object, int flags)
 {
        if (object->classId == RelationRelationId)
@@ -1356,8 +1357,10 @@ AcquireDeletionLock(const ObjectAddress *object, int flags)
 
 /*
  * ReleaseDeletionLock - release an object deletion lock
+ *
+ * Companion to AcquireDeletionLock.
  */
-static void
+void
 ReleaseDeletionLock(const ObjectAddress *object)
 {
        if (object->classId == RelationRelationId)

diff --git a/src/backend/catalog/pg_shdepend.c b/src/backend/catalog/pg_shdepend.c
index faf42b76405f483570d47ae99a15deef0c5c976b..ca705190efc3fb789882e075858ab895eec706e7 100644 (file)
--- a/src/backend/catalog/pg_shdepend.c
+++ b/src/backend/catalog/pg_shdepend.c
@@ -1240,7 +1240,10 @@ shdepDropOwned(List *roleids, DropBehavior behavior)
                                                                                        sdepForm->objid);
                                        break;
                                case SHARED_DEPENDENCY_POLICY:
-                                       /* If unable to remove role from policy, remove policy. */
+                                       /*
+                                        * Try to remove role from policy; if unable to, remove
+                                        * policy.
+                                        */
                                        if (!RemoveRoleFromObjectPolicy(roleid,
                                                                                                        sdepForm->classid,
                                                                                                        sdepForm->objid))
@@ -1248,6 +1251,18 @@ shdepDropOwned(List *roleids, DropBehavior behavior)
                                                obj.classId = sdepForm->classid;
                                                obj.objectId = sdepForm->objid;
                                                obj.objectSubId = sdepForm->objsubid;
+                                               /*
+                                                * Acquire lock on object, then verify this dependency
+                                                * is still relevant.  If not, the object might have
+                                                * been dropped or the policy modified.  Ignore the
+                                                * object in that case.
+                                                */
+                                               AcquireDeletionLock(&obj, 0);
+                                               if (!systable_recheck_tuple(scan, tuple))
+                                               {
+                                                       ReleaseDeletionLock(&obj);
+                                                       break;
+                                               }
                                                add_exact_object_address(&obj, deleteobjs);
                                        }
                                        break;
@@ -1258,6 +1273,13 @@ shdepDropOwned(List *roleids, DropBehavior behavior)
                                                obj.classId = sdepForm->classid;
                                                obj.objectId = sdepForm->objid;
                                                obj.objectSubId = sdepForm->objsubid;
+                                               /* as above */
+                                               AcquireDeletionLock(&obj, 0);
+                                               if (!systable_recheck_tuple(scan, tuple))
+                                               {
+                                                       ReleaseDeletionLock(&obj);
+                                                       break;
+                                               }
                                                add_exact_object_address(&obj, deleteobjs);
                                        }
                                        break;

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index f138e61a8d3d9d111e52b5fe59742df1967e9042..655ce2dc4bb0f036b95f43d0245e99a1c773bf34 100644 (file)
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -899,7 +899,6 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
        if (slotname)
                PreventInTransactionBlock(isTopLevel, "DROP SUBSCRIPTION");
 
-
        ObjectAddressSet(myself, SubscriptionRelationId, subid);
        EventTriggerSQLDropAddObject(&myself, true, true);
 

diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h
index ff06c9aa77788ef05a38e4d53adfa7b333541f6b..ec79e2c7dc2cc25405584c0ed525549c389db7a3 100644 (file)
--- a/src/include/catalog/dependency.h
+++ b/src/include/catalog/dependency.h
@@ -195,6 +195,10 @@ typedef enum ObjectClass
 
 /* in dependency.c */
 
+extern void AcquireDeletionLock(const ObjectAddress *object, int flags);
+
+extern void ReleaseDeletionLock(const ObjectAddress *object);
+
 extern void performDeletion(const ObjectAddress *object,
                                DropBehavior behavior, int flags);