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

> On May 16, 2026, at 07:18, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
>
> 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;
>>
>> Assert(conninfo_needed);
>> ```
>>
>> So, for those two paths, if check_pub_rdt is true, then the Assert
>> will be fired, is that intentional?
>
> I've fixed it to be Assert(new_conninfo || orig_conninfo_needed).
>
> 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=false when one of:
>
> ALTER SUBSCRIPTION ... SERVER
> ALTER SUBSCRIPTION ... CONNECTION
> ALTER SUBSCRIPTION ... SET (slot_name=NONE)
>
> and not try to be precise about which other settings might need
> check_pub_rdt or not.

Yep, this part looks good now.

>
> 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?
>

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 = NONE) to disassociate it from the slot.
```

The error message already says that the problem is “user mapping not found”, 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/