Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server

On Wed, Apr 22, 2026 at 11:52 AM Chao Li <li.evan.chao@gmail.com> wrote:
>
> Hi,
>
> The comment explicitly says to skip ACL checks on the old server because it will be removed anyway.
>
> But after looking into GetSubscription(), I found that when the aclcheck parameter is false, it still calls ForeignServerConnectionString(). I think that is the root cause of the bug.
>
> To fix this, I worked out a solution that stores the server OID in Subscription, and only calls ForeignServerConnectionString()lazily when sub->conninfo is actually needed. I also added a new test case to cover this scenario. Without the fix, the new test fails.
> See attached patch for details.
>
Hi Li,

Thanks for the patch.
Some comments:

1.
             if (aclresult != ACLCHECK_OK)
                 ereport(ERROR,
-                        (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-                         errmsg("subscription owner \"%s\" does not
have permission on foreign server \"%s\"",
-                                GetUserNameFromId(subform->subowner, false),
-                                server->servername)));
+                        errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+                        errmsg("subscription owner \"%s\" does not
have permission on foreign server \"%s\"",
+                               GetUserNameFromId(subform->subowner, false),
+                               server->servername));
+            sub->conninfo = ForeignServerConnectionString(subform->subowner,
+                                                          server);
         }

Add a new line before the call to ForeignServerConnectionString(),
also I think you should put the if condition within curly brackets
because it spans more than one line and might confuse developers while
adding new code.

2. I think you should add a comment in the function header above
GetSubscription() stating that if aclcheck is false, then the conninfo
will be set to null and users need to call GetSubscriptionConnInfo to
get the conninfo.

3.
 Datum
 test_fdw_connection(PG_FUNCTION_ARGS)
 {
+    GetUserMapping(PG_GETARG_OID(0), PG_GETARG_OID(1));
     PG_RETURN_TEXT_P(cstring_to_text("dbname=regress_doesnotexist
user=doesnotexist password=secret"));
 }

Add a comment above this change describing why it's required.

regards,
Ajin Cherian
Fujitsu Australia