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

On Tue, 2026-05-26 at 17:44 -0700, Amit Kapila wrote:
> > Yep, this part looks good now.

Thank you. I committed 0001 with one additional fix: make sure that
ALTER SUBSCRIPTION DISABLE doesn't try to construct the old conninfo.

I plan to commit 0002 soon.

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

OK.

> > 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.
> > 
> 
> Right, the hint doesn't sound to be a right solution for the problem
> reported.

The user might no longer have permissions on the server to create a new
user mapping, so if they want to drop the subscription, then they need
to set slot_name=NONE.

In other words, we can't delete the existing hint language; we can only
add new language about adding the user mapping back. I don't see a
great way to add that language without making it too long and
confusing, but I am open to suggestion.

v7-0003 is more than just an error message change, so I updated the
commit message. It handles the case where the drop path has
rstates!=NIL but slot_name=NONE: without 0003, the drop will fail while
trying to construct the conninfo (without the HINT); with 0003, the
drop will silently succeed without removing the tablesync slots (as it
does with a connection failure when subconninfo is set).

If the use of a subtransaction is fine there, then I think we should
proceed with 0003 and whatever hint message is agreeable.

Regards,
	Jeff Davis