MIME-Version: 1.0
References: <D908370F-2695-4231-851D-17179A6A6F2A@gmail.com>
In-Reply-To: <D908370F-2695-4231-851D-17179A6A6F2A@gmail.com>
From: Ajin Cherian <itsajin@gmail.com>
Date: Wed, 29 Apr 2026 14:44:47 +1000
Message-ID: 
 <CAFPTHDZk-eY1K6OGWhLSWJ0C7qncVQvaxUmBL2r69_1CiznfTA@mail.gmail.com>
Subject: Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken
 old server
To: Chao Li <li.evan.chao@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAFPTHDZk-eY1K6OGWhLSWJ0C7qncVQvaxUmBL2r69_1CiznfTA%40mail.gmail.com>
Precedence: bulk

On Wed, Apr 22, 2026 at 11:52=E2=80=AFAM Chao Li <li.evan.chao@gmail.com> w=
rote:
>
> 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 Subscr=
iption, and only calls ForeignServerConnectionString()lazily when sub->conn=
info is actually needed. I also added a new test case to cover this scenari=
o. Without the fix, the new test fails.
> See attached patch for details.
>
Hi Li,

Thanks for the patch.
Some comments:

1.
             if (aclresult !=3D 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 =3D ForeignServerConnectionString(subform->subow=
ner,
+                                                          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=3Dregress_doesnotexist
user=3Ddoesnotexist password=3Dsecret"));
 }

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

regards,
Ajin Cherian
Fujitsu Australia