Avoid errors during DROP SUBSCRIPTION when slot_name is NONE.

Previously, if the subscription used a server,
ForeignServerConnectionString() could raise an error (e.g. missing
user mapping) during DROP SUBSCRIPTION even if the conninfo wasn't
needed at all.

Construct conninfo after the early return, so that if slot_name is
NONE and rstates is NIL, the DROP SUBSCRIPTION will succeed even if
ForeignServerConnectionString() raises an error (e.g. missing user
mapping).

If slot_name is NONE and rstates is not NIL, DROP SUBSCRIPTION may
still encounter an error from ForeignServerConnectionString().

Reported-by: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/OS9PR01MB12149B54DEA148108C6FA5667F52D2@OS9PR01MB12149.jpnprd01.prod.outlook.com

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index c9faf68cbc5b4ac39850fdb8a6dfc980f3d2923d..ee06a726f420d3f14d1df853e14fece6b0022f18 100644 (file)
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2260,6 +2260,43 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
        return myself;
 }
 
+/*
+ * Construct conninfo from a subscription's server. Like libpqrcv_connect(),
+ * if an error occurs, set *err to the error message and return NULL.
+ *
+ * However, failures in ForeignServerConnectionString() may ereport(ERROR),
+ * and (also like libpqrcv_connect) it's not worth adding the machinery to
+ * pass all of those back to the caller just to cover this one case.
+ */
+static char *
+construct_subserver_conninfo(Oid subserver, Oid subowner, char **err)
+{
+       AclResult       aclresult;
+       ForeignServer *server;
+
+       *err = NULL;
+
+       server = GetForeignServer(subserver);
+
+       aclresult = object_aclcheck(ForeignServerRelationId, subserver,
+                                                               subowner, ACL_USAGE);
+       if (aclresult != ACLCHECK_OK)
+       {
+               /*
+                * Unable to generate connection string because permissions on the
+                * foreign server have been removed. Follow the same logic as an
+                * unusable subconninfo (which will result in an ERROR later unless
+                * slot_name = NONE).
+                */
+               *err = psprintf(_("subscription owner \"%s\" does not have permission on foreign server \"%s\""),
+                                               GetUserNameFromId(subowner, false),
+                                               server->servername);
+               return NULL;
+       }
+
+       return ForeignServerConnectionString(subowner, server);
+}
+
 /*
  * Drop a subscription
  */
@@ -2271,10 +2308,12 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
        HeapTuple       tup;
        Oid                     subid;
        Oid                     subowner;
+       Oid                     subserver;
+       char       *subconninfo = NULL;
        Datum           datum;
        bool            isnull;
        char       *subname;
-       char       *conninfo;
+       char       *conninfo = NULL;
        char       *slotname;
        List       *subworkers;
        ListCell   *lc;
@@ -2313,9 +2352,15 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
                return;
        }
 
+       datum = SysCacheGetAttr(SUBSCRIPTIONOID, tup,
+                                                       Anum_pg_subscription_subconninfo, &isnull);
+       if (!isnull)
+               subconninfo = TextDatumGetCString(datum);
+
        form = (Form_pg_subscription) GETSTRUCT(tup);
        subid = form->oid;
        subowner = form->subowner;
+       subserver = form->subserver;
        must_use_password = !superuser_arg(subowner) && form->subpasswordrequired;
 
        /* must be owner */
@@ -2337,39 +2382,6 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
                                                                   Anum_pg_subscription_subname);
        subname = pstrdup(NameStr(*DatumGetName(datum)));
 
-       /* Get conninfo */
-       if (OidIsValid(form->subserver))
-       {
-               AclResult       aclresult;
-               ForeignServer *server;
-
-               server = GetForeignServer(form->subserver);
-               aclresult = object_aclcheck(ForeignServerRelationId, form->subserver,
-                                                                       form->subowner, ACL_USAGE);
-               if (aclresult != ACLCHECK_OK)
-               {
-                       /*
-                        * Unable to generate connection string because permissions on the
-                        * foreign server have been removed. Follow the same logic as an
-                        * unusable subconninfo (which will result in an ERROR later
-                        * unless slot_name = NONE).
-                        */
-                       err = psprintf(_("subscription owner \"%s\" does not have permission on foreign server \"%s\""),
-                                                  GetUserNameFromId(form->subowner, false),
-                                                  server->servername);
-                       conninfo = NULL;
-               }
-               else
-                       conninfo = ForeignServerConnectionString(form->subowner,
-                                                                                                        server);
-       }
-       else
-       {
-               datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
-                                                                          Anum_pg_subscription_subconninfo);
-               conninfo = TextDatumGetCString(datum);
-       }
-
        /* Get slotname */
        datum = SysCacheGetAttr(SUBSCRIPTIONOID, tup,
                                                        Anum_pg_subscription_subslotname, &isnull);
@@ -2506,6 +2518,11 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
         */
        load_file("libpqwalreceiver", false);
 
+       if (OidIsValid(subserver))
+               conninfo = construct_subserver_conninfo(subserver, subowner, &err);
+       else
+               conninfo = subconninfo;
+
        if (conninfo)
                wrconn = walrcv_connect(conninfo, true, true, must_use_password,
                                                                subname, &err);

diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 61236672ce454c4165a17d2608ad76e2849ab412..8dbfac66326b3666e6eb0df4f5a982d751a9203d 100644 (file)
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -213,11 +213,12 @@ DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
 BEGIN;
 ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret';
 ABORT;
-CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server OPTIONS(user 'foo', password 'secret');
+-- fails, cannot drop slot
+DROP SUBSCRIPTION regress_testsub6;
+ERROR:  user mapping not found for user "regress_subscription_user3", server "test_server"
 ALTER SUBSCRIPTION regress_testsub6 DISABLE;
 ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
 DROP SUBSCRIPTION regress_testsub6; --ok
-DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
 SET SESSION AUTHORIZATION regress_subscription_user;
 REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3;
 DROP SERVER test_server;

diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 665b510f1805ffc0c919afe05e23c21d7c1792e7..05533d66675bc0f12b413ba53ad0001917eeb958 100644 (file)
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -164,14 +164,13 @@ BEGIN;
 ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret';
 ABORT;
 
-CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server OPTIONS(user 'foo', password 'secret');
+-- fails, cannot drop slot
+DROP SUBSCRIPTION regress_testsub6;
 
 ALTER SUBSCRIPTION regress_testsub6 DISABLE;
 ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
 DROP SUBSCRIPTION regress_testsub6; --ok
 
-DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
-
 SET SESSION AUTHORIZATION regress_subscription_user;
 REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3;