Content-Type: text/plain;
	charset=us-ascii
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: <f13a8e29410bbbf9999290f2c04513a8884fa51c.camel@j-davis.com>
Date: Fri, 15 May 2026 15:18:31 +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: <1B3A7FF0-3292-484F-992C-27D172772EB8@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>
To: Jeff Davis <pgsql@j-davis.com>
Archived-At: 
 <https://www.postgresql.org/message-id/1B3A7FF0-3292-484F-992C-27D172772EB8%40gmail.com>
Precedence: bulk


> On May 15, 2026, at 05:45, Jeff Davis <pgsql@j-davis.com> wrote:
>=20
> Hi,
>=20
> I added a fix to the series: v5-0001 fixes check_pub_rdt for the
> foreign server case.
>=20
> On Sat, 2026-05-09 at 11:08 +0800, Chao Li wrote:
>> Ah, I see. You added a new conninfo_needed parameter to
>> GetSubscription(), which separates the decision of building conninfo
>> from the ACL check. Cool, I believe this is a better approach.
>>=20
>> So 0001 looks good to me. nitpick is that conninfo_aclcheck is now
>> only meaningful when conninfo_needed is true. I wonder if we should
>> mention that briefly in the function header comment, or add an
>> assertion such as: Assert(conninfo_needed || !conninfo_aclcheck); to
>> avoid possible misuse of conninfo_aclcheck in the future.
>=20
> I refactored the fix in v5-0002 to do this in a more organized way: =
now
> all option parsing happens first, so I can more precisely decide which
> paths need conninfo and which ones don't.
>=20
>> So with 0002, if slotname is NULL but rstates is not NIL, it looks
>> possible that we no longer build conninfo and therefore skip the
>> cleanup on the publisher side.
>=20
> I separated this into two patches:
>=20
> v5-0003 just moves the connection string building after the early =
exit,
> so that if slotname is NONE and rstates is NIL, then it won't try to
> build the connection string at all (and therefore won't get an error
> while doing so).
>=20
> v5-0004 fixes the remaining issue when slotname is NONE and rstates is
> *not* NIL. It uses a subtransaction to catch the error, so that the
> DROP TRANSACTION will still succeed even though it can't connect to =
the
> publisher to drop the tablesync slots. This feels a bit over-
> engineered, but it does maintain the expected behavior in this case. =
It
> also routes errors inside of ForeignServerConnectionString() through
> ReportSlotConnectionError(), which adds a helpful hint.
>=20
> Regards,
> Jeff Davis
>=20
>=20
>=20
>=20
> =
<v5-0001-Check-retain_dead_tuples-for-ALTER-SUBSCRIPTION-..patch><v5-0002-=
Avoid-errors-during-ALTER-SUBSCRIPTION.patch><v5-0003-Avoid-errors-during-=
DROP-SUBSCRIPTION-when-slot_n.patch><v5-0004-DROP-SUBSCRIPTION-improve-err=
or-message.patch>

I have just one comment on v5:

In 0002, for both ALTER_SUBSCRIPTION_SERVER and =
ALTER_SUBSCRIPTION_CONNECTION, conninfo_needed is false:
```
	if (stmt->kind =3D=3D ALTER_SUBSCRIPTION_SERVER ||
		stmt->kind =3D=3D ALTER_SUBSCRIPTION_CONNECTION)
	{
		conninfo_needed =3D false;
	}
```

0001 adds "check_pub_rdt =3D sub->retaindeadtuples;" to the both paths:

0002 adds this Assert:
```
	if (update_failover || update_two_phase || check_pub_rdt)
	{
		bool		must_use_password;
		char	   *err;
		WalReceiverConn *wrconn;

		Assert(conninfo_needed);
```

So, for those two paths, if check_pub_rdt is true, then the Assert will =
be fired, is that intentional?

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