> 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()lazilywhen 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/
