> On Apr 29, 2026, at 12:44, Ajin Cherian <[email protected]> wrote: > > 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.
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/
v2-0001-Allow-altering-subscription-server-connection-wit.patch
Description: Binary data
