public inbox for [email protected]  

help / color / mirror / Atom feed

From: Ajin Cherian <[email protected]>
To: Chao Li <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
Date: Wed, 29 Apr 2026 14:44:47 +1000
Message-ID: <CAFPTHDZk-eY1K6OGWhLSWJ0C7qncVQvaxUmBL2r69_1CiznfTA@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>

On Wed, Apr 22, 2026 at 11:52 AM Chao Li <[email protected]> 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

view thread (28+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected]
  Subject: Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
  In-Reply-To: <CAFPTHDZk-eY1K6OGWhLSWJ0C7qncVQvaxUmBL2r69_1CiznfTA@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox