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

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.

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?

Regards,
Jeff Davis