Feedback-ID: ie3de48e3:Fastmail
Date: Tue, 8 Sep 2026 09:50:02 +0200
From: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de>
To: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>,
	shveta malik <shveta.malik@gmail.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Remove stale XXX comment in logical launcher
Message-ID: <ap-9M0MfJ0M72mnu@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="3lvjw6xwjq6anblz"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <TY4PR01MB17718CEAE3E330A24936393B194B22@TY4PR01MB17718.jpnprd01.prod.outlook.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ap-9M0MfJ0M72mnu%40alvherre.pgsql>
Precedence: bulk


--3lvjw6xwjq6anblz
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

Hello,

On 2026-Sep-07, Zhijie Hou (Fujitsu) wrote:

> Here is the updated patch with the suggested comments.

Mumble.

How about like this?

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

--3lvjw6xwjq6anblz
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename=0001-Remove-stale-XXX-comment-in-logical-launcher.patch

From 277a9d6fdb7e22ee58ab1557e4227a52e01bb45a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Tue, 8 Sep 2026 09:44:36 +0200
Subject: [PATCH] Remove stale XXX comment in logical launcher

An XXX comment in launcher.c questioned why the slot drop does not request
disabling logical decoding. But the conflict detection slot is a physical slot,
so there is no logical decoding state to disable. Remove the comment and add an
explanation atop the slot creation function clarifying that the slot is physical.

Author: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
Reviewed-by: shveta malik <shveta.malik@gmail.com>
Discussion: https://postgr.es/m/TY4PR01MB177186C8AB596F30DF704ECDC94B62@TY4PR01MB17718.jpnprd01.prod.outlook.com
---
 src/backend/replication/logical/launcher.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/src/backend/replication/logical/launcher.c b/src/backend/replication/logical/launcher.c
index c328ad53ecc..5057a928adb 100644
--- a/src/backend/replication/logical/launcher.c
+++ b/src/backend/replication/logical/launcher.c
@@ -1284,12 +1284,12 @@ ApplyLauncherMain(Datum main_arg)
 				retain_dead_tuples = true;
 
 				/*
-				 * Create a replication slot to retain information necessary
-				 * for conflict detection such as dead tuples, commit
-				 * timestamps, and origins.
+				 * Create a (physical) replication slot to retain information
+				 * necessary for conflict detection such as dead tuples,
+				 * commit timestamps, and origins.
 				 *
 				 * The slot is created before starting the apply worker to
-				 * prevent it from unnecessarily maintaining its
+				 * prevent the worker from unnecessarily maintaining its
 				 * oldest_nonremovable_xid.
 				 *
 				 * The slot is created even for a disabled subscription to
@@ -1418,7 +1418,6 @@ ApplyLauncherMain(Datum main_arg)
 		if (MyReplicationSlot)
 		{
 			if (!retain_dead_tuples)
-				/* XXX unclear why we don't request logical decoding disable */
 				ReplicationSlotDropAcquired(false);
 			else if (can_update_xmin)
 				update_conflict_slot_xmin(xmin);
@@ -1627,6 +1626,11 @@ reset_conflict_slot_xmin_to_safe_horizon(void)
 /*
  * Create and acquire the replication slot used to retain information for
  * conflict detection, if not yet.
+ *
+ * A physical slot is enough, as no logical decoding is going to be performed
+ * through it.  In fact, the slot will only be used through its xmin horizon
+ * to prevent the removal of dead tuples and commit timestamp data required by
+ * subscriptions with retain_dead_tuples enabled in any database.
  */
 void
 CreateConflictDetectionSlot(void)
-- 
2.47.3


--3lvjw6xwjq6anblz--