Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Avoid errors during DROP SUBSCRIPTION when slot_name is NONE.

  2. Avoid errors during ALTER SUBSCRIPTION.

  3. Check retain_dead_tuples for ALTER SUBSCRIPTION ... SERVER.

  4. Allow multiple xacts during table sync in logical replication.

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

    Chao Li <li.evan.chao@gmail.com> — 2026-04-22T01:51:36Z

    Hi,

I tested the v19 new feature CREATE SUBSCRIPTION ... SERVER yesterday, and found an issue: once the old server becomes broken, the subscription cannot be recovered by switching it to a good server.

Here is a repro:
```
evantest=# CREATE EXTENSION postgres_fdw;
CREATE EXTENSION

# Create two servers
evantest=# CREATE SERVER old_srv FOREIGN DATA WRAPPER postgres_fdw
evantest-# OPTIONS (host '127.0.0.1', dbname 'postgres', port '5432');
CREATE SERVER
evantest=# CREATE SERVER new_srv FOREIGN DATA WRAPPER postgres_fdw
evantest-# OPTIONS (host '127.0.0.1', dbname 'postgres', port '5432');
CREATE SERVER
evantest=# CREATE USER MAPPING FOR CURRENT_USER SERVER old_srv
evantest-# OPTIONS (user 'dummy', password 'dummy');
CREATE USER MAPPING
evantest=# CREATE USER MAPPING FOR CURRENT_USER SERVER new_srv
evantest-# OPTIONS (user 'dummy', password 'dummy');
CREATE USER MAPPING

# Add old_server to a subscription
evantest=# CREATE SUBSCRIPTION sub_bug SERVER old_srv
evantest-# PUBLICATION pub_does_not_matter
evantest-# WITH (connect = false, slot_name = NONE);
WARNING:  subscription was created, but is not connected
HINT:  To initiate replication, you must manually create the replication slot, enable the subscription, and alter the subscription to refresh publications.
CREATE SUBSCRIPTION

# Break old_srv
evantest=# DROP USER MAPPING FOR CURRENT_USER SERVER old_srv;
DROP USER MAPPING

# Fail to switch to a good server because old_srv is broken
evantest=# ALTER SUBSCRIPTION sub_bug SERVER new_srv;
ERROR:  user mapping not found for user "chaol", server "old_srv"
evantest=# ALTER SUBSCRIPTION sub_bug CONNECTION 'host=127.0.0.1 dbname=postgres port=5432 user=dummy password=dummy';
ERROR:  user mapping not found for user "chaol", server "old_srv"
```

In AlterSubscription(), this comment made me think this is a bug:
```
	/*
	 * Skip ACL checks on the subscription's foreign server, if any. If
	 * changing the server (or replacing it with a raw connection), then the
	 * old one will be removed anyway. If changing something unrelated,
	 * there's no need to do an additional ACL check here; that will be done
	 * by the subscription worker anyway.
	 */
	sub = GetSubscription(subid, false, false);
```

The comment explicitly says to skip ACL checks on the old server because it will be removed anyway.

But after looking into GetSubscription(), I found that when the aclcheck parameter is false, it still calls ForeignServerConnectionString(). I think that is the root cause of the bug.

To fix this, I worked out a solution that stores the server OID in Subscription, and only calls ForeignServerConnectionString()lazily when sub->conninfo is actually needed. I also added a new test case to cover this scenario. Without the fix, the new test fails.
See attached patch for details.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

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

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2026-04-22T12:35:02Z

    Dear Chao,

> I tested the v19 new feature CREATE SUBSCRIPTION ... SERVER yesterday, and
> found an issue: once the old server becomes broken, the subscription cannot be
> recovered by switching it to a good server.

Thanks for testing. I could reproduce the same issue. In addition to yours, I found
DROP SUBSCRIPTION cannot be done anymore. To switch the connection or drop it,
I had to create the same user mapping must be created again.

```
postgres=# DROP SUBSCRIPTION sub_bug ;
ERROR:  user mapping not found for user "postgres", server "old_srv"
postgres=# CREATE USER MAPPING FOR CURRENT_USER SERVER old_srv
OPTIONS (user 'dummy', password 'dummy');
CREATE USER MAPPING
postgres=# DROP SUBSCRIPTION sub_bug ;
DROP SUBSCRIPTION
```

Before deep dive to your fix, I'm unclear why dropping the active USER MAPPING is
allowed. Personally, it should be avoided anyway. Do you know why it's not restricted?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

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

    Chao Li <li.evan.chao@gmail.com> — 2026-04-23T03:37:42Z

    
> On Apr 22, 2026, at 20:35, Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> wrote:
> 
> Dear Chao,
> 
>> I tested the v19 new feature CREATE SUBSCRIPTION ... SERVER yesterday, and
>> found an issue: once the old server becomes broken, the subscription cannot be
>> recovered by switching it to a good server.
> 
> Thanks for testing. I could reproduce the same issue. In addition to yours, I found
> DROP SUBSCRIPTION cannot be done anymore. To switch the connection or drop it,
> I had to create the same user mapping must be created again.
> 
> ```
> postgres=# DROP SUBSCRIPTION sub_bug ;
> ERROR:  user mapping not found for user "postgres", server "old_srv"
> postgres=# CREATE USER MAPPING FOR CURRENT_USER SERVER old_srv
> OPTIONS (user 'dummy', password 'dummy');
> CREATE USER MAPPING
> postgres=# DROP SUBSCRIPTION sub_bug ;
> DROP SUBSCRIPTION
> ```
> 
> Before deep dive to your fix, I'm unclear why dropping the active USER MAPPING is
> allowed. Personally, it should be avoided anyway. Do you know why it's not restricted?
> 
> Best regards,
> Hayato Kuroda
> FUJITSU LIMITED
> 

Hi Hayato-san,

There is an existing test case in subscription.sql:
```
-- fail, must connect but lacks USAGE on server, as well as user mapping
DROP SUBSCRIPTION regress_testsub6;
```

So, I guess that’s an intentional behavior. You have to fix the broken server or switch to a good one before dropping the subscription. That’s my understanding from the test cases.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

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

    Ajin Cherian <itsajin@gmail.com> — 2026-04-29T04:44:47Z

    On Wed, Apr 22, 2026 at 11:52 AM Chao Li <li.evan.chao@gmail.com> wrote:
>
> Hi,
>
> The comment explicitly says to skip ACL checks on the old server because it will be removed anyway.
>
> But after looking into GetSubscription(), I found that when the aclcheck parameter is false, it still calls ForeignServerConnectionString(). I think that is the root cause of the bug.
>
> To fix this, I worked out a solution that stores the server OID in Subscription, and only calls ForeignServerConnectionString()lazily when sub->conninfo is actually needed. I also added a new test case to cover this scenario. Without the fix, the new test fails.
> See attached patch for details.
>
Hi Li,

Thanks for the patch.
Some comments:

1.
             if (aclresult != ACLCHECK_OK)
                 ereport(ERROR,
-                        (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-                         errmsg("subscription owner \"%s\" does not
have permission on foreign server \"%s\"",
-                                GetUserNameFromId(subform->subowner, false),
-                                server->servername)));
+                        errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+                        errmsg("subscription owner \"%s\" does not
have permission on foreign server \"%s\"",
+                               GetUserNameFromId(subform->subowner, false),
+                               server->servername));
+            sub->conninfo = ForeignServerConnectionString(subform->subowner,
+                                                          server);
         }

Add a new line before the call to ForeignServerConnectionString(),
also I think you should put the if condition within curly brackets
because it spans more than one line and might confuse developers while
adding new code.

2. I think you should add a comment in the function header above
GetSubscription() stating that if aclcheck is false, then the conninfo
will be set to null and users need to call GetSubscriptionConnInfo to
get the conninfo.

3.
 Datum
 test_fdw_connection(PG_FUNCTION_ARGS)
 {
+    GetUserMapping(PG_GETARG_OID(0), PG_GETARG_OID(1));
     PG_RETURN_TEXT_P(cstring_to_text("dbname=regress_doesnotexist
user=doesnotexist password=secret"));
 }

Add a comment above this change describing why it's required.

regards,
Ajin Cherian
Fujitsu Australia

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

    Chao Li <li.evan.chao@gmail.com> — 2026-04-30T04:11:47Z

    
> On Apr 29, 2026, at 12:44, Ajin Cherian <itsajin@gmail.com> wrote:
> 
> On Wed, Apr 22, 2026 at 11:52 AM Chao Li <li.evan.chao@gmail.com> wrote:
>> 
>> Hi,
>> 
>> The comment explicitly says to skip ACL checks on the old server because it will be removed anyway.
>> 
>> But after looking into GetSubscription(), I found that when the aclcheck parameter is false, it still calls ForeignServerConnectionString(). I think that is the root cause of the bug.
>> 
>> To fix this, I worked out a solution that stores the server OID in Subscription, and only calls ForeignServerConnectionString()lazily when sub->conninfo is actually needed. I also added a new test case to cover this scenario. Without the fix, the new test fails.
>> See attached patch for details.
>> 
> Hi Li,
> 
> Thanks for the patch.

Thanks for your review.

> Some comments:
> 
> 1.
>             if (aclresult != ACLCHECK_OK)
>                 ereport(ERROR,
> -                        (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> -                         errmsg("subscription owner \"%s\" does not
> have permission on foreign server \"%s\"",
> -                                GetUserNameFromId(subform->subowner, false),
> -                                server->servername)));
> +                        errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> +                        errmsg("subscription owner \"%s\" does not
> have permission on foreign server \"%s\"",
> +                               GetUserNameFromId(subform->subowner, false),
> +                               server->servername));
> +            sub->conninfo = ForeignServerConnectionString(subform->subowner,
> +                                                          server);
>         }
> 
> Add a new line before the call to ForeignServerConnectionString(),

Okay.

> also I think you should put the if condition within curly brackets
> because it spans more than one line and might confuse developers while
> adding new code.

I don’t think curly brackets are needed as there is only one statement under the if though the statement spans multiple lines. There are plenty of examples in the existing code, for example:
```
	if (!wrconn)
		ereport(ERROR,
				errcode(ERRCODE_CONNECTION_FAILURE),
				errmsg("subscription \"%s\" could not connect to the publisher: %s",
					   sub->name, err));
```

> 
> 2. I think you should add a comment in the function header above
> GetSubscription() stating that if aclcheck is false, then the conninfo
> will be set to null and users need to call GetSubscriptionConnInfo to
> get the conninfo.

Updated the header comment.

> 
> 3.
> Datum
> test_fdw_connection(PG_FUNCTION_ARGS)
> {
> +    GetUserMapping(PG_GETARG_OID(0), PG_GETARG_OID(1));
>     PG_RETURN_TEXT_P(cstring_to_text("dbname=regress_doesnotexist
> user=doesnotexist password=secret"));
> }
> 
> Add a comment above this change describing why it's required.
> 

Added the comment.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

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

    Ajin Cherian <itsajin@gmail.com> — 2026-05-01T03:58:51Z

    On Thu, Apr 30, 2026 at 2:12 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> >
> > 2. I think you should add a comment in the function header above
> > GetSubscription() stating that if aclcheck is false, then the conninfo
> > will be set to null and users need to call GetSubscriptionConnInfo to
> > get the conninfo.
>
> Updated the header comment.

 /*
  * Fetch the subscription from the syscache.
+ *
+ * If missing_ok is false, throw an error if the subscription is not found.
+ * If true, return NULL in that case.
+ *
+ * If aclcheck is true, check whether the subscription owner has permission on
+ * the subscription's foreign server, and load the connection string from the
+ * foreign server. Later, GetSubscriptionConnInfo() should be called to get
+ * the connection string.
  */
 Subscription *
 GetSubscription(Oid subid, bool missing_ok, bool aclcheck)

I don't think this comment is right. If aclcheck is true, users need
not call GetSubscriptionConnInfo() to get the connection string, as it
is populated in this function itself. It is only if aclecheck is
false, do callers need to do that.
Isn't that the case? There are multiple places in the apply worker
where GetSubscription() is called with aclcheck is true and
GetSubscriptionConnInfo() is not called there.

regards,
Ajin Cherian
Fujitsu Australia

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

    Zsolt Parragi <zsolt.parragi@percona.com> — 2026-05-05T20:53:20Z

    Hello

- server = GetForeignServer(subform->subserver);
+ server = GetForeignServer(sub->serverid);

Couldn't we also move this inside the if?

+/*
+ * Return the subscription's connection string, loading it into the
+ * subscription memory context if necessary.
+ *
+ * GetSubscription must be called earlier to set sub->serverid, because ACL
+ * checks are performed there.
+ */
+char *
+GetSubscriptionConnInfo(Subscription *sub)

This is related to Ajin's comment earlier, the part about ACL check
seems incorrect to me.

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

    Chao Li <li.evan.chao@gmail.com> — 2026-05-06T07:47:21Z

    
> On May 1, 2026, at 11:58, Ajin Cherian <itsajin@gmail.com> wrote:
> 
> On Thu, Apr 30, 2026 at 2:12 PM Chao Li <li.evan.chao@gmail.com> wrote:
>> 
>> 
>> 
>>> 
>>> 2. I think you should add a comment in the function header above
>>> GetSubscription() stating that if aclcheck is false, then the conninfo
>>> will be set to null and users need to call GetSubscriptionConnInfo to
>>> get the conninfo.
>> 
>> Updated the header comment.
> 
> /*
>  * Fetch the subscription from the syscache.
> + *
> + * If missing_ok is false, throw an error if the subscription is not found.
> + * If true, return NULL in that case.
> + *
> + * If aclcheck is true, check whether the subscription owner has permission on
> + * the subscription's foreign server, and load the connection string from the
> + * foreign server. Later, GetSubscriptionConnInfo() should be called to get
> + * the connection string.
>  */
> Subscription *
> GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
> 
> I don't think this comment is right. If aclcheck is true, users need
> not call GetSubscriptionConnInfo() to get the connection string, as it
> is populated in this function itself. It is only if aclecheck is
> false, do callers need to do that.
> Isn't that the case? There are multiple places in the apply worker
> where GetSubscription() is called with aclcheck is true and
> GetSubscriptionConnInfo() is not called there.
> 
> regards,
> Ajin Cherian
> Fujitsu Australia

Hi Ajin,

Thank you for the comment. After rereading that part, I agree the wording is not clear.

What I meant is that GetSubscriptionConnInfo() is a safe accessor, if sub->conninfo has already been resolved, it just returns it; otherwise, it resolves it on demand. So the intended usage is that callers can consistently use GetSubscriptionConnInfo() when they need the connection string.

I also missed doing a broader search for direct uses of sub->conninfo and replacing them with GetSubscriptionConnInfo() where appropriate. Sorry about that.

I will address both issues in v3.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

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

    Chao Li <li.evan.chao@gmail.com> — 2026-05-06T07:57:51Z

    
> On May 6, 2026, at 04:53, Zsolt Parragi <zsolt.parragi@percona.com> wrote:
> 
> Hello
> 
> - server = GetForeignServer(subform->subserver);
> + server = GetForeignServer(sub->serverid);
> 
> Couldn't we also move this inside the if?

Ah, true. Both aclresult and server can be moved into the if.

> 
> +/*
> + * Return the subscription's connection string, loading it into the
> + * subscription memory context if necessary.
> + *
> + * GetSubscription must be called earlier to set sub->serverid, because ACL
> + * checks are performed there.
> + */
> +char *
> +GetSubscriptionConnInfo(Subscription *sub)
> 
> This is related to Ajin's comment earlier, the part about ACL check
> seems incorrect to me.

Yes, see my reply to Ajin in the previous email.

PFA v3 - addressed Ajin and Zsolt’s comments.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

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

    Jeff Davis <pgsql@j-davis.com> — 2026-05-09T01:01:28Z

    On Wed, 2026-05-06 at 15:57 +0800, Chao Li wrote:
> PFA v3 - addressed Ajin and Zsolt’s comments.

Thank you for the report!

The proposed patch seems unnecessarily complex, though. It seems too
easy to add GetSubscriptionConninfo() in the wrong place and end up
with another problem that's not easily detected.

Can't we just do something like the attached? It's easy to explain at
the call site that, when changing to a different server or using
CONNECTION instead, that we don't need the old conninfo at all.

I included your test case in my patch, and it passes.

Also, Hayato Kuroda's report was an issue also because the error could
be thrown even if slotname was NULL. Patch attached for that, as well.
Thank you, also!

Regards,
	Jeff Davis

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

    Chao Li <li.evan.chao@gmail.com> — 2026-05-09T03:08:28Z

    
> On May 9, 2026, at 09:01, Jeff Davis <pgsql@j-davis.com> wrote:
> 
> On Wed, 2026-05-06 at 15:57 +0800, Chao Li wrote:
>> PFA v3 - addressed Ajin and Zsolt’s comments.
> 
> Thank you for the report!
> 
> The proposed patch seems unnecessarily complex, though. It seems too
> easy to add GetSubscriptionConninfo() in the wrong place and end up
> with another problem that's not easily detected.
> 
> Can't we just do something like the attached? It's easy to explain at
> the call site that, when changing to a different server or using
> CONNECTION instead, that we don't need the old conninfo at all.
> 
> I included your test case in my patch, and it passes.
> 
> Also, Hayato Kuroda's report was an issue also because the error could
> be thrown even if slotname was NULL. Patch attached for that, as well.
> Thank you, also!
> 
> Regards,
> Jeff Davis
> 
> <v4-0001-Avoid-errors-during-ALTER-SUBSCRIPTION.patch><v4-0002-Avoid-errors-during-DROP-SUBSCRIPTION.patch>

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.

For 0002, I have a doubt. Now conninfo is built only when slotname is not NULL. But after reading through DropSubscription(), I am not sure conninfo is strictly tied to slotname.

For example, this fast path returns only when both slotname is NULL and rstates is NIL:
```
	/*
	 * If there is no slot associated with the subscription, we can finish
	 * here.
	 */
	if (!slotname && rstates == NIL)
	{
		table_close(rel, NoLock);
		return;
	}
```

That seems to imply that even when slotname is NULL, rstates might still be not NIL.

Later, if conninfo is not NULL, the code connects to the publisher and does some cleanup work for tablesync slots:
```
	if (conninfo)
		wrconn = walrcv_connect(conninfo, true, true, must_use_password,
								subname, &err);
        ...
			/*
			 * Drop the tablesync slots associated with removed tables.
			 *
			 * For SYNCDONE/READY states, the tablesync slot is known to have
			 * already been dropped by the tablesync worker.
			 *
			 * For other states, there is no certainty, maybe the slot does
			 * not exist yet. Also, if we fail after removing some of the
			 * slots, next time, it will again try to drop already dropped
			 * slots and fail. For these reasons, we allow missing_ok = true
			 * for the drop.
			 */
			if (rstate->state != SUBREL_STATE_SYNCDONE)
			{
				char		syncslotname[NAMEDATALEN] = {0};

				ReplicationSlotNameForTablesync(subid, relid, syncslotname,
												sizeof(syncslotname));
				ReplicationSlotDropAtPubNode(wrconn, syncslotname, true);
			}
```

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.

Best reagards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

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

    Jeff Davis <pgsql@j-davis.com> — 2026-05-14T21:45:10Z

    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

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

    Chao Li <li.evan.chao@gmail.com> — 2026-05-15T07:18:31Z

    
> On May 15, 2026, at 05:45, Jeff Davis <pgsql@j-davis.com> wrote:
> 
> 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
> 
> 
> 
> 
> <v5-0001-Check-retain_dead_tuples-for-ALTER-SUBSCRIPTION-..patch><v5-0002-Avoid-errors-during-ALTER-SUBSCRIPTION.patch><v5-0003-Avoid-errors-during-DROP-SUBSCRIPTION-when-slot_n.patch><v5-0004-DROP-SUBSCRIPTION-improve-error-message.patch>

I have just one comment on v5:

In 0002, for both ALTER_SUBSCRIPTION_SERVER and ALTER_SUBSCRIPTION_CONNECTION, conninfo_needed is false:
```
	if (stmt->kind == ALTER_SUBSCRIPTION_SERVER ||
		stmt->kind == ALTER_SUBSCRIPTION_CONNECTION)
	{
		conninfo_needed = false;
	}
```

0001 adds "check_pub_rdt = sub->retaindeadtuples;" to the both paths:

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?

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

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

    Jeff Davis <pgsql@j-davis.com> — 2026-05-15T23:18:43Z

    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

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

    Chao Li <li.evan.chao@gmail.com> — 2026-05-18T05:28:32Z

    
> On May 16, 2026, at 07:18, Jeff Davis <pgsql@j-davis.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/

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

    Amit Kapila <amit.kapila16@gmail.com> — 2026-05-27T00:44:29Z

    On Sun, May 17, 2026 at 10:29 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
> > On May 16, 2026, at 07:18, Jeff Davis <pgsql@j-davis.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.
>

Right, the hint doesn't sound to be a right solution for the problem reported.

-- 
With Regards,
Amit Kapila.

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

    Jeff Davis <pgsql@j-davis.com> — 2026-06-17T20:26:51Z

    On Wed, 2026-04-22 at 12:35 +0000, Hayato Kuroda (Fujitsu) wrote:
> Before deep dive to your fix, I'm unclear why dropping the active
> USER MAPPING is
> allowed. Personally, it should be avoided anyway. Do you know why
> it's not restricted?

We don't record dependencies on user mappings because technically it's
not dependent on a specific user mapping. That is, GetUserMapping() can
fall back to the public user mapping.

Regards,
	Jeff Davis

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

    Jeff Davis <pgsql@j-davis.com> — 2026-06-17T23:10:41Z

    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

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

    Jeff Davis <pgsql@j-davis.com> — 2026-06-19T18:40:34Z

    On Wed, 2026-06-17 at 16:10 -0700, Jeff Davis wrote:
> 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.

0002 committed.

For 0003, I don't think we can absorb every kind of error that might
happen inside fdwconnection. libpqrcv_connect() doesn't attempt to do
so, so neither should construct_subserver_conninfo().

What 0003 is really trying to avoid is fairly normal kinds of errors
that can happen on a non-broken FDW that get in the way of dropping a
subscription. That includes a missing user mapping or a failed ACL
check. But aside from that, it's hard to think of other examples we'd
clearly want to absorb.

Options:

  * check for those two error codes explicitly
  * try to be more systematic about what kinds of errors should be
absorbed or not
  * add an escape hatch for users to turn off tablesync slots so that
DROP will always succeed
  * consider it an unimportant edge case and leave it the way it is
(with 0001 & 0002 already done), and close the open item

Thoughts?

Regards,
	Jeff Davis

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

    Jeff Davis <pgsql@j-davis.com> — 2026-06-22T21:10:25Z

    On Fri, 2026-06-19 at 11:40 -0700, Jeff Davis wrote:
>   * add an escape hatch for users to turn off tablesync slots so that
> DROP will always succeed

It looks like SLOT_NAME=NONE is already supposed to be this escape
hatch, even for tablesync slots. From the docs:

"To proceed in this situation, first disable the subscription by
executing ALTER SUBSCRIPTION ... DISABLE, and then disassociate it from
the replication slot by executing ALTER SUBSCRIPTION ... SET (slot_name
= NONE). After that, DROP SUBSCRIPTION will no longer attempt any
actions on a remote host."

https://www.postgresql.org/docs/devel/sql-dropsubscription.html

But DropSubscription() only does the early-return if there are no
tablesync slots. If there are tablesync slots, it still tries to
contact the publisher, even if SLOT_NAME=NONE.

>   * consider it an unimportant edge case and leave it the way it is
> (with 0001 & 0002 already done), and close the open item

I plan to close this open item, and treat the above as a pre-existing
bug, which may require a backport.

Regards,
	Jeff Davis

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

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2026-06-24T02:18:20Z

    Dear Jeff,

I went through the 0003 patch. Apart from HINT message, I found that error code
ERRCODE_CONNECTION_FAILURE is always used. Should we report the original errcode
of the root cause? Or it's OK because the observed outcome is the cannot-connect
error?

Another comment:


```
+               conninfo = ForeignServerConnectionString(subowner, server);
+               Assert(conninfo != NULL);
```

Do we have to consider the case that ForeignServerConnectionString() returns NULL
without any ERRORs? For the production build, the Assert() would be skipped and
the segmentation fault would happen in the `if (!conninfo)` part.
I checked the doc [1] but there are no specifications for it.

[1]: https://www.postgresql.org/docs/devel/sql-createforeigndatawrapper.html#:~:text=ereport(ERROR)%20function.-,CONNECTION%20connection_function,-connection_function%20is%20the

Best regards,
Hayato Kuroda
FUJITSU LIMITED

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

    Amit Kapila <amit.kapila16@gmail.com> — 2026-06-29T04:27:54Z

    On Tue, Jun 23, 2026 at 2:40 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Fri, 2026-06-19 at 11:40 -0700, Jeff Davis wrote:
> >   * add an escape hatch for users to turn off tablesync slots so that
> > DROP will always succeed
>
> It looks like SLOT_NAME=NONE is already supposed to be this escape
> hatch, even for tablesync slots. From the docs:
>
> "To proceed in this situation, first disable the subscription by
> executing ALTER SUBSCRIPTION ... DISABLE, and then disassociate it from
> the replication slot by executing ALTER SUBSCRIPTION ... SET (slot_name
> = NONE). After that, DROP SUBSCRIPTION will no longer attempt any
> actions on a remote host."
>
> https://www.postgresql.org/docs/devel/sql-dropsubscription.html
>
> But DropSubscription() only does the early-return if there are no
> tablesync slots. If there are tablesync slots, it still tries to
> contact the publisher, even if SLOT_NAME=NONE.
>

AFAICS, the corresponding DropSubscription() code is as follows:

if (!slotname && rstates == NIL)
{
table_close(rel, NoLock);
return;
}
....
if (OidIsValid(subserver))
conninfo = construct_subserver_conninfo(subserver, subowner, &err);
else
conninfo = subconninfo;

if (conninfo)
wrconn = walrcv_connect(conninfo, true, true, must_use_password,
subname, &err);

if (wrconn == NULL)
{
if (!slotname)
{
/* be tidy */
list_free(rstates);
table_close(rel, NoLock);
return;
}
else
{
ReportSlotConnectionError(rstates, subid, slotname, err);
}
}

So, if the connection is not successful then we simply return without
checking tablesync slots (when slotname is NONE) whereas with FDW
server connection, the ERROR will be raised from
construct_subserver_conninfo->ForeignServerConnectionString(). Isn't
that different from current situation?

> >   * consider it an unimportant edge case and leave it the way it is
> > (with 0001 & 0002 already done), and close the open item
>
> I plan to close this open item, and treat the above as a pre-existing
> bug, which may require a backport.
>

As per my understanding it is better to go with your fix idea even
though we don't have better ideas for HINT message. I see your point
of making HINT much longer as we may need to add something like (...
or re-create the user mapping/ACL and retry..), so we can keep the
current one as it is and if we see any real user complaint then we can
consider improving it in the future.

-- 
With Regards,
Amit Kapila.

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

    Amit Kapila <amit.kapila16@gmail.com> — 2026-06-29T04:37:00Z

    On Wed, Jun 24, 2026 at 7:48 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> I went through the 0003 patch. Apart from HINT message, I found that error code
> ERRCODE_CONNECTION_FAILURE is always used. Should we report the original errcode
> of the root cause?
>

That could be marginally better but not sure if it is worth the
complexity. I have a few more points regarding 0003:
* Regarding the point: "we can't absorb every kind of error." In the
PG_CATCH, re-throw query-cancel / interrupt-class conditions (if those
are possible) instead of swallowing them, otherwise a SIGINT during
ForeignServerConnectionString() becomes a silent successful DROP (for
slot_name=NONE) or a mislabeled connection error.

*
+ if (aclresult != ACLCHECK_OK)
+ ereport(ERROR,
+ errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("subscription owner \"%s\" does not have permission on
foreign server \"%s\"",
+    GetUserNameFromId(subowner, false),
+    server->servername));

I am not sure if this is a good idea as the outer catch will anyway
silence this.

-- 
With Regards,
Amit Kapila.

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

    Jeff Davis <pgsql@j-davis.com> — 2026-07-06T23:41:34Z

    On Mon, 2026-06-29 at 10:07 +0530, Amit Kapila wrote:
> On Wed, Jun 24, 2026 at 7:48 AM Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> > 
> > I went through the 0003 patch. Apart from HINT message, I found
> > that error code
> > ERRCODE_CONNECTION_FAILURE is always used. Should we report the
> > original errcode
> > of the root cause?
> > 
> 
> That could be marginally better but not sure if it is worth the
> complexity. I have a few more points regarding 0003:
> * Regarding the point: "we can't absorb every kind of error." In the
> PG_CATCH, re-throw query-cancel / interrupt-class conditions (if
> those
> are possible) instead of swallowing them, otherwise a SIGINT during
> ForeignServerConnectionString() becomes a silent successful DROP (for
> slot_name=NONE) or a mislabeled connection error.

The DROP SUBSCRIPTION documentation says that:

"To proceed in this situation, first disable the subscription by
executing ALTER SUBSCRIPTION ... DISABLE, and then disassociate it from
the replication slot by executing ALTER SUBSCRIPTION ... SET (slot_name
= NONE). After that, DROP SUBSCRIPTION will no longer attempt any
actions on a remote host."

But that stopped being true after commit ce0fdbfe97. There should be
some kind of escape such that we can force a DROP SUBSCRIPTION to
succeed without trying to connect to the remote host. And if we have
that escape, then we don't need to overcomplicate this by using a
subtransaction and trying to guess which errors are recoverable or not.

Regards,
	Jeff Davis

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

    Jeff Davis <pgsql@j-davis.com> — 2026-07-06T23:46:10Z

    On Mon, 2026-06-29 at 09:57 +0530, Amit Kapila wrote:
> wrconn = walrcv_connect(conninfo, true, true, must_use_password,
> subname, &err);

...

> So, if the connection is not successful then we simply return without
> checking tablesync slots (when slotname is NONE) whereas with FDW
> server connection, the ERROR will be raised from
> construct_subserver_conninfo->ForeignServerConnectionString(). Isn't
> that different from current situation?

It's easier to cause an error in ForeignServerConnectionString(); but
walrcv_connect() can throw errors, too. Also, a network connection is a
side-effect that you may not want to have for whatever reason.

I think it's better to have an escape that prevents any connection to
the publisher, as the documentation says, to deal with these kinds of
edge cases.

Regards,
	Jeff Davis

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

    Amit Kapila <amit.kapila16@gmail.com> — 2026-07-08T04:59:15Z

    On Tue, Jul 7, 2026 at 5:11 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Mon, 2026-06-29 at 10:07 +0530, Amit Kapila wrote:
> > On Wed, Jun 24, 2026 at 7:48 AM Hayato Kuroda (Fujitsu)
> > <kuroda.hayato@fujitsu.com> wrote:
> > >
> > > I went through the 0003 patch. Apart from HINT message, I found
> > > that error code
> > > ERRCODE_CONNECTION_FAILURE is always used. Should we report the
> > > original errcode
> > > of the root cause?
> > >
> >
> > That could be marginally better but not sure if it is worth the
> > complexity. I have a few more points regarding 0003:
> > * Regarding the point: "we can't absorb every kind of error." In the
> > PG_CATCH, re-throw query-cancel / interrupt-class conditions (if
> > those
> > are possible) instead of swallowing them, otherwise a SIGINT during
> > ForeignServerConnectionString() becomes a silent successful DROP (for
> > slot_name=NONE) or a mislabeled connection error.
>
> The DROP SUBSCRIPTION documentation says that:
>
> "To proceed in this situation, first disable the subscription by
> executing ALTER SUBSCRIPTION ... DISABLE, and then disassociate it from
> the replication slot by executing ALTER SUBSCRIPTION ... SET (slot_name
> = NONE). After that, DROP SUBSCRIPTION will no longer attempt any
> actions on a remote host."
>
> But that stopped being true after commit ce0fdbfe97. There should be
> some kind of escape such that we can force a DROP SUBSCRIPTION to
> succeed without trying to connect to the remote host. And if we have
> that escape, then we don't need to overcomplicate this by using a
> subtransaction and trying to guess which errors are recoverable or not.
>

Okay, I see your point and we can make it return early when slot_name
is NONE. However, do we need to consider a case where users want to
manually manage the subscription's main slot, so it uses
slot_name=NONE? I mean users want to manually maintain the lifecycle
of the slot and due to that even during the CREATE SUBSCRIPTION, it
uses the specified slot_name instead of the default. But still
tablesync slots need to be removed in such a case as those are
internally created. Actually, this best-case try to connect to
publisher works for such cases. I think this is mostly a theoretical
scenario because ideally these tablesync slots should be removed at
the end of tablesync especially when users are doing drop subscription
as part of planned operation, so we can as well rely on the document
as we are doing now.

-- 
With Regards,
Amit Kapila.