> On May 9, 2026, at 09:01, Jeff Davis <[email protected]> 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/
