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

On Thu, Apr 30, 2026 at 2:12 PM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>
>
>
> >
> > 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.

/*
* Fetch the subscription from the syscache.
+ *
+ * If missing_ok is false, throw an error if the subscription is not found.
+ * If true, return NULL in that case.
+ *
+ * If aclcheck is true, check whether the subscription owner has permission on
+ * the subscription's foreign server, and load the connection string from the
+ * foreign server. Later, GetSubscriptionConnInfo() should be called to get
+ * the connection string.
*/
Subscription *
GetSubscription(Oid subid, bool missing_ok, bool aclcheck)

I don't think this comment is right. If aclcheck is true, users need
not call GetSubscriptionConnInfo() to get the connection string, as it
is populated in this function itself. It is only if aclecheck is
false, do callers need to do that.
Isn't that the case? There are multiple places in the apply worker
where GetSubscription() is called with aclcheck is true and
GetSubscriptionConnInfo() is not called there.

regards,
Ajin Cherian
Fujitsu Australia