Horton

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

Chao Li <li.evan.chao@gmail.com>

View thread
From: Chao Li <li.evan.chao@gmail.com>
To: Ajin Cherian <itsajin@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2026-05-06T07:47:21Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Check retain_dead_tuples for ALTER SUBSCRIPTION ... SERVER.


> On May 1, 2026, at 11:58, Ajin Cherian <itsajin@gmail.com> wrote:
> 
> On Thu, Apr 30, 2026 at 2:12 PM Chao Li <li.evan.chao@gmail.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

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/