Improve DROP SERVER handling of dependent subscriptions.

We do not allow a DROP SERVER ... CASCADE to implicitly drop a
subscription, because it's in a shared catalog and dropping a
subscription has side effects. Instead we throw an error and the user
must drop the subscription explicitly. Document this behavior and add
a HINT to the error message.

Generalize AcquireDeletionLock()/ReleaseDeletionLock() to use shared
object locks for all shared catalogs, which includes AuthMemRelationId
and now SubscriptionRelationId.

Move error message after AcquireDeletionLock() to avoid an unnecessary
error if there's a concurrent DROP SUBSCRIPTION.

Addresses finding 10 & 15 in report from linked discussion.

Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20260710195902.4f.noahmisch@microsoft.com
Backpatch-through: 19

diff --git a/doc/src/sgml/ref/drop_server.sgml b/doc/src/sgml/ref/drop_server.sgml
index f83a661b3ebd9490cfe1dbb47813ff4ff08a8485..5fa0b763f36f7c16433de4f6b7ef2f2becec85e9 100644 (file)
--- a/doc/src/sgml/ref/drop_server.sgml
+++ b/doc/src/sgml/ref/drop_server.sgml
@@ -66,6 +66,10 @@ DROP SERVER [ IF EXISTS ] <replaceable class="parameter">name</replaceable> [, .
       user mappings),
       and in turn all objects that depend on those objects
       (see <xref linkend="ddl-depend"/>).
+      However, a subscription that uses the server is never dropped
+      automatically; it must be dropped with
+      <link linkend="sql-dropsubscription"><command>DROP SUBSCRIPTION</command></link>
+      before the server can be dropped.
      </para>
     </listitem>
    </varlistentry>

diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 52cd2caf9d49b8f15b25eb3ce8153fc23677b7ed..c8dd78341ebb6617d030a9b76065043d5ded8e0a 100644 (file)
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -900,17 +900,6 @@ findDependentObjects(const ObjectAddress *object,
                        object->objectSubId == 0)
                        continue;
 
-               /*
-                * Check that the dependent object is not in a shared catalog, which
-                * is not supported by doDeletion().
-                */
-               if (IsSharedRelation(otherObject.classId))
-                       ereport(ERROR,
-                                       (errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST),
-                                        errmsg("cannot drop %s because %s depends on it",
-                                                       getObjectDescription(object, false),
-                                                       getObjectDescription(&otherObject, false))));
-
                /*
                 * Must lock the dependent object before recursing to it.
                 */
@@ -931,6 +920,22 @@ findDependentObjects(const ObjectAddress *object,
                        continue;
                }
 
+               /*
+                * Check that the dependent object is not in a shared catalog, which
+                * is not supported by doDeletion().
+                */
+               if (IsSharedRelation(otherObject.classId))
+               {
+                       char       *otherObjDesc = getObjectDescription(&otherObject,
+                                                                                                                       false);
+
+                       ereport(ERROR,
+                                       (errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST),
+                                        errmsg("cannot drop %s because %s depends on it",
+                                                       getObjectDescription(object, false), otherObjDesc),
+                                        errhint("Drop %s first.", otherObjDesc)));
+               }
+
                /*
                 * We do need to delete it, so identify objflags to be passed down,
                 * which depend on the dependency type.
@@ -1579,7 +1584,7 @@ AcquireDeletionLock(const ObjectAddress *object, int flags)
                else
                        LockRelationOid(object->objectId, AccessExclusiveLock);
        }
-       else if (object->classId == AuthMemRelationId)
+       else if (IsSharedRelation(object->classId))
                LockSharedObject(object->classId, object->objectId, 0,
                                                 AccessExclusiveLock);
        else
@@ -1600,7 +1605,7 @@ ReleaseDeletionLock(const ObjectAddress *object)
 {
        if (object->classId == RelationRelationId)
                UnlockRelationOid(object->objectId, AccessExclusiveLock);
-       else if (object->classId == AuthMemRelationId)
+       else if (IsSharedRelation(object->classId))
                UnlockSharedObject(object->classId, object->objectId, 0,
                                                   AccessExclusiveLock);
        else

diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 1bb785f4f9f5fb3b1989d54261427906cc90224f..259db7473344ccb00b1499f83249e97029a668d6 100644 (file)
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -205,6 +205,10 @@ ALTER FOREIGN DATA WRAPPER test_fdw CONNECTION test_fdw_connection;
 WARNING:  changing the foreign-data wrapper connection function can cause the options for dependent objects to become invalid
 DROP USER MAPPING FOR regress_subscription_user2 SERVER test_server;
 REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user2;
+-- fail, subscription depends on the server and cannot be dropped by CASCADE
+DROP SERVER test_server CASCADE;
+ERROR:  cannot drop server test_server because subscription regress_testsub6 depends on it
+HINT:  Drop subscription regress_testsub6 first.
 REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
 SET SESSION AUTHORIZATION regress_subscription_user3;
 -- ok, lacks USAGE on test_server, but replacing connection anyway

diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index f19740fdfb8381a1ca6bbfbcad0e7d6d2a4d1341..7718c742974cf3a8d30513e9a7388328870190e6 100644 (file)
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -150,6 +150,8 @@ ALTER SUBSCRIPTION regress_testsub6 OWNER TO regress_subscription_user2;
 ALTER FOREIGN DATA WRAPPER test_fdw CONNECTION test_fdw_connection;
 DROP USER MAPPING FOR regress_subscription_user2 SERVER test_server;
 REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user2;
+-- fail, subscription depends on the server and cannot be dropped by CASCADE
+DROP SERVER test_server CASCADE;
 
 REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
 SET SESSION AUTHORIZATION regress_subscription_user3;