Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.400.21\))
Subject: Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken
 old server
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: 
 <CAFPTHDamW43eQTpZGfUhZAhYxCwYSmCs2ww3sxjBoHDvv7Oawg@mail.gmail.com>
Date: Wed, 6 May 2026 15:47:21 +0800
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <013CFA76-B42A-4351-8698-0B293BABDAE3@gmail.com>
References: <D908370F-2695-4231-851D-17179A6A6F2A@gmail.com>
 <CAFPTHDZk-eY1K6OGWhLSWJ0C7qncVQvaxUmBL2r69_1CiznfTA@mail.gmail.com>
 <960DD2C4-A22C-463A-90ED-86E0FABD8D20@gmail.com>
 <CAFPTHDamW43eQTpZGfUhZAhYxCwYSmCs2ww3sxjBoHDvv7Oawg@mail.gmail.com>
To: Ajin Cherian <itsajin@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/013CFA76-B42A-4351-8698-0B293BABDAE3%40gmail.com>
Precedence: bulk


> On May 1, 2026, at 11:58, Ajin Cherian <itsajin@gmail.com> wrote:
>=20
> On Thu, Apr 30, 2026 at 2:12=E2=80=AFPM Chao Li =
<li.evan.chao@gmail.com> wrote:
>>=20
>>=20
>>=20
>>>=20
>>> 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.
>>=20
>> Updated the header comment.
>=20
> /*
>  * 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)
>=20
> 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.
>=20
> regards,
> Ajin Cherian
> Fujitsu Australia

Hi Ajin,

Thank you for the comment. After rereading that part, I agree the =
wording is not clear.

What I meant is that GetSubscriptionConnInfo() is a safe accessor, if =
sub->conninfo has already been resolved, it just returns it; otherwise, =
it resolves it on demand. So the intended usage is that callers can =
consistently use GetSubscriptionConnInfo() when they need the connection =
string.

I also missed doing a broader search for direct uses of sub->conninfo =
and replacing them with GetSubscriptionConnInfo() where appropriate. =
Sorry about that.

I will address both issues in v3.

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