public inbox for [email protected]  

help / color / mirror / Atom feed

From: Chao Li <[email protected]>
To: Jeff Davis <[email protected]>
Cc: Zsolt Parragi <[email protected]>
Cc: Hayato Kuroda (Fujitsu) <[email protected]>
Cc: Ajin Cherian <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
Date: Sat, 9 May 2026 11:08:28 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<CAFPTHDZk-eY1K6OGWhLSWJ0C7qncVQvaxUmBL2r69_1CiznfTA@mail.gmail.com>
	<[email protected]>
	<CAN4CZFMemmQaxs9qT8MW3u_xRXrOa9mtmyup3Nizwb0V_C1axw@mail.gmail.com>
	<[email protected]>
	<[email protected]>



> On May 9, 2026, at 09:01, Jeff Davis <[email protected]> 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/

view thread (28+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox