> On May 16, 2026, at 07:18, Jeff Davis <[email protected]> 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/
