Horton

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. Check retain_dead_tuples for ALTER SUBSCRIPTION ... SERVER.

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

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

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

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

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

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

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

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

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

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

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