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

> On Apr 29, 2026, at 12:44, Ajin Cherian <itsajin@gmail.com> wrote:
> 
> 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.

Thanks for your review.

> 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(),

Okay.

> 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.

I don’t think curly brackets are needed as there is only one statement under the if though the statement spans multiple lines. There are plenty of examples in the existing code, for example:
```
	if (!wrconn)
		ereport(ERROR,
				errcode(ERRCODE_CONNECTION_FAILURE),
				errmsg("subscription \"%s\" could not connect to the publisher: %s",
					   sub->name, err));
```

> 
> 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.

Updated the header comment.

> 
> 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.
> 

Added the comment.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/