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: <65166eb6feb87356f8d60bfe646289ce15a6c729.camel@j-davis.com>
Date: Mon, 18 May 2026 13:28:32 +0800
Cc: Zsolt Parragi <zsolt.parragi@percona.com>,
 "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>,
 Ajin Cherian <itsajin@gmail.com>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <24C6472D-DCAB-4F69-AE8A-FFB4AAB3F6F0@gmail.com>
References: <D908370F-2695-4231-851D-17179A6A6F2A@gmail.com>
 <CAFPTHDZk-eY1K6OGWhLSWJ0C7qncVQvaxUmBL2r69_1CiznfTA@mail.gmail.com>
 <960DD2C4-A22C-463A-90ED-86E0FABD8D20@gmail.com>
 <CAN4CZFMemmQaxs9qT8MW3u_xRXrOa9mtmyup3Nizwb0V_C1axw@mail.gmail.com>
 <8B38AFCA-7FFA-44CF-B9A8-81BDCEAFB224@gmail.com>
 <b03eb0fecb7df88cd3eb2acf48bd1230e28a2923.camel@j-davis.com>
 <705148C3-519C-45E7-9EA8-48D9F3B79B06@gmail.com>
 <f13a8e29410bbbf9999290f2c04513a8884fa51c.camel@j-davis.com>
 <1B3A7FF0-3292-484F-992C-27D172772EB8@gmail.com>
 <65166eb6feb87356f8d60bfe646289ce15a6c729.camel@j-davis.com>
To: Jeff Davis <pgsql@j-davis.com>
Archived-At: 
 <https://www.postgresql.org/message-id/24C6472D-DCAB-4F69-AE8A-FFB4AAB3F6F0%40gmail.com>
Precedence: bulk


> On May 16, 2026, at 07:18, Jeff Davis <pgsql@j-davis.com> wrote:
>=20
> On Fri, 2026-05-15 at 15:18 +0800, Chao Li wrote:
>> 0002 adds this Assert:
>> ```
>> if (update_failover || update_two_phase || check_pub_rdt)
>> {
>> bool must_use_password;
>> char    *err;
>> WalReceiverConn *wrconn;
>>=20
>> Assert(conninfo_needed);
>> ```
>>=20
>> So, for those two paths, if check_pub_rdt is true, then the Assert
>> will be fired, is that intentional?
>=20
> I've fixed it to be Assert(new_conninfo || orig_conninfo_needed).
>=20
> Also, the code above was missing the case of SUBOPT_ORIGIN which could
> set check_pub_rdt. I changed it to be more conservative and set
> orig_conninfo_needed=3Dfalse when one of:
>=20
>  ALTER SUBSCRIPTION ... SERVER
>  ALTER SUBSCRIPTION ... CONNECTION
>  ALTER SUBSCRIPTION ... SET (slot_name=3DNONE)
>=20
> and not try to be precise about which other settings might need
> check_pub_rdt or not.

Yep, this part looks good now.

>=20
> What do you think of v6-0003? Is it over-engineered? Should the
> subtransaction happen at a lower level? Is there an alternative to
> using a subtransaction?
>=20

For the reason you described in the commit message, catching the error =
and later reporting it through ReportSlotConnectionError(), I don't =
think this is over-engineered. I also think keeping the subtransaction =
inside construct_subserver_conninfo() is reasonable, because this =
error-capturing behavior seems specific to the DROP SUBSCRIPTION path.

As for whether the HINT itself really helps, I would reserve my opinion. =
As the test output shows:
```
DROP SUBSCRIPTION regress_testsub6;
ERROR:  could not connect to publisher when attempting to drop =
replication slot "dummy": user mapping not found for user =
"regress_subscription_user3", server "test_server"
HINT:  Use ALTER SUBSCRIPTION ... DISABLE to disable the subscription, =
and then use ALTER SUBSCRIPTION ... SET (slot_name =3D NONE) to =
disassociate it from the slot.
```

The error message already says that the problem is =E2=80=9Cuser mapping =
not found=E2=80=9D, so fixing the user mapping could also be a solution. =
So, the HINT is still useful, but it might not be the most direct fix in =
some case.

I got another small comment. Now construct_subserver_conninfo() has some =
duplicate code block of getting foreign server, aclcheck and =
ForeignServerConnectionString as what GetSubscription() has, maybe we =
can wrap that piece of code into a helper function.

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