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

> On May 9, 2026, at 09:01, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
>
> On Wed, 2026-05-06 at 15:57 +0800, Chao Li wrote:
>> PFA v3 - addressed Ajin and Zsolt’s comments.
>
> Thank you for the report!
>
> 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.
>
> 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.
>
> I included your test case in my patch, and it passes.
>
> 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!
>
> Regards,
> Jeff Davis
>
> <v4-0001-Avoid-errors-during-ALTER-SUBSCRIPTION.patch><v4-0002-Avoid-errors-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 == 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 = 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 = true
* for the drop.
*/
if (rstate->state != SUBREL_STATE_SYNCDONE)
{
char syncslotname[NAMEDATALEN] = {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/