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

Hi,

I added a fix to the series: v5-0001 fixes check_pub_rdt for the
foreign server case.

On Sat, 2026-05-09 at 11:08 +0800, Chao Li wrote:
> 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.

I refactored the fix in v5-0002 to do this in a more organized way: now
all option parsing happens first, so I can more precisely decide which
paths need conninfo and which ones don't.

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

I separated this into two patches:

v5-0003 just moves the connection string building after the early exit,
so that if slotname is NONE and rstates is NIL, then it won't try to
build the connection string at all (and therefore won't get an error
while doing so).

v5-0004 fixes the remaining issue when slotname is NONE and rstates is
*not* NIL. It uses a subtransaction to catch the error, so that the
DROP TRANSACTION will still succeed even though it can't connect to the
publisher to drop the tablesync slots. This feels a bit over-
engineered, but it does maintain the expected behavior in this case. It
also routes errors inside of ForeignServerConnectionString() through
ReportSlotConnectionError(), which adds a helpful hint.

Regards,
Jeff Davis