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: Fri, 15 May 2026 15:18:31 +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]>
	<[email protected]>
	<[email protected]>



> On May 15, 2026, at 05:45, Jeff Davis <[email protected]> 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/

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