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: <b03eb0fecb7df88cd3eb2acf48bd1230e28a2923.camel@j-davis.com>
Date: Sat, 9 May 2026 11:08:28 +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: <705148C3-519C-45E7-9EA8-48D9F3B79B06@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>
To: Jeff Davis <pgsql@j-davis.com>
Archived-At: 
 <https://www.postgresql.org/message-id/705148C3-519C-45E7-9EA8-48D9F3B79B06%40gmail.com>
Precedence: bulk


> On May 9, 2026, at 09:01, Jeff Davis <pgsql@j-davis.com> wrote:
>=20
> On Wed, 2026-05-06 at 15:57 +0800, Chao Li wrote:
>> PFA v3 - addressed Ajin and Zsolt=E2=80=99s comments.
>=20
> Thank you for the report!
>=20
> The proposed patch seems unnecessarily complex, though. It seems too
> easy to add GetSubscriptionConninfo() in the wrong place and end up
> with another problem that's not easily detected.
>=20
> Can't we just do something like the attached? It's easy to explain at
> the call site that, when changing to a different server or using
> CONNECTION instead, that we don't need the old conninfo at all.
>=20
> I included your test case in my patch, and it passes.
>=20
> Also, Hayato Kuroda's report was an issue also because the error could
> be thrown even if slotname was NULL. Patch attached for that, as well.
> Thank you, also!
>=20
> Regards,
> Jeff Davis
>=20
> =
<v4-0001-Avoid-errors-during-ALTER-SUBSCRIPTION.patch><v4-0002-Avoid-error=
s-during-DROP-SUBSCRIPTION.patch>

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.

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.

For 0002, I have a doubt. Now conninfo is built only when slotname is =
not NULL. But after reading through DropSubscription(), I am not sure =
conninfo is strictly tied to slotname.

For example, this fast path returns only when both slotname is NULL and =
rstates is NIL:
```
	/*
	 * If there is no slot associated with the subscription, we can =
finish
	 * here.
	 */
	if (!slotname && rstates =3D=3D NIL)
	{
		table_close(rel, NoLock);
		return;
	}
```

That seems to imply that even when slotname is NULL, rstates might still =
be not NIL.

Later, if conninfo is not NULL, the code connects to the publisher and =
does some cleanup work for tablesync slots:
```
	if (conninfo)
		wrconn =3D walrcv_connect(conninfo, true, true, =
must_use_password,
								subname, =
&err);
        ...
			/*
			 * Drop the tablesync slots associated with =
removed tables.
			 *
			 * For SYNCDONE/READY states, the tablesync slot =
is known to have
			 * already been dropped by the tablesync worker.
			 *
			 * For other states, there is no certainty, =
maybe the slot does
			 * not exist yet. Also, if we fail after =
removing some of the
			 * slots, next time, it will again try to drop =
already dropped
			 * slots and fail. For these reasons, we allow =
missing_ok =3D true
			 * for the drop.
			 */
			if (rstate->state !=3D SUBREL_STATE_SYNCDONE)
			{
				char		=
syncslotname[NAMEDATALEN] =3D {0};

				ReplicationSlotNameForTablesync(subid, =
relid, syncslotname,
										=
		sizeof(syncslotname));
				ReplicationSlotDropAtPubNode(wrconn, =
syncslotname, true);
			}
```

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.

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