Hi, On Mon, Jan 5, 2026 at 1:25 PM Hayato Kuroda (Fujitsu) <[email protected]> wrote: > > Dear Amit, > > > > > I think it is better if we add some comments atop > > ReplicationSlotAlter() as you are suggesting. What do you think of the > > attached? > > Thanks for attaching the patch. There is a small typo: > > > + * clinet-side. Enabling it at any random point during decoding has the > > "clinet" should be client. Others are OK for me. >
Thank you all for your comments! I agree with suggested fixes. Please, see them in the attached patch. On Mon, Jan 5, 2026 at 2:56 PM Chao Li <[email protected]> wrote: > > Hi Amit, > > While reviewing your change, I find the other typo in slot.c: > ``` > - /* Check if the slot exits with the given name. */ > + /* Check if the slot exists with the given name. */ > s = SearchNamedReplicationSlot(name, false); > if (s == NULL || !s->in_use) > ``` > > “Exits” and “exists” have totally different meanings, thus might lead to > misunderstanding. Attached is a trivial diff to fix that. > Good catch! I am not sure that we should put both fixes together, so I'll put your fix in a separate patch. -- Best regards, Daniil Davydov
From ea061b3f16a9449fd49d4188dde9620d7992ebbf Mon Sep 17 00:00:00 2001 From: Daniil Davidov <[email protected]> Date: Mon, 5 Jan 2026 17:17:56 +0700 Subject: [PATCH v2 2/2] Improve comments for ReplicationSlotAcquire --- src/backend/replication/slot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index ee520aa9433..f8f0c1e2fc0 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -629,7 +629,7 @@ retry: LWLockAcquire(ReplicationSlotControlLock, LW_SHARED); - /* Check if the slot exits with the given name. */ + /* Check if the slot exists with the given name. */ s = SearchNamedReplicationSlot(name, false); if (s == NULL || !s->in_use) { -- 2.43.0
From e955164e14cff2f67727a0144056610426dcc825 Mon Sep 17 00:00:00 2001 From: Daniil Davidov <[email protected]> Date: Mon, 5 Jan 2026 17:15:15 +0700 Subject: [PATCH v2 1/2] Improve comments for ReplicationSlotAlter --- src/backend/replication/slot.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index 75d26fa61ea..ee520aa9433 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -369,13 +369,7 @@ IsSlotForConflictCheck(const char *name) * name: Name of the slot * db_specific: logical decoding is db specific; if the slot is going to * be used for that pass true, otherwise false. - * two_phase: Allows decoding of prepared transactions. We allow this option - * to be enabled only at the slot creation time. If we allow this option - * to be changed during decoding then it is quite possible that we skip - * prepare first time because this option was not enabled. Now next time - * during getting changes, if the two_phase option is enabled it can skip - * prepare because by that time start decoding point has been moved. So the - * user will only get commit prepared. + * two_phase: If enabled, allows decoding of prepared transactions. * failover: If enabled, allows the slot to be synced to standbys so * that logical replication can be resumed after failover. * synced: True if the slot is synchronized from the primary server. @@ -940,6 +934,16 @@ ReplicationSlotDrop(const char *name, bool nowait) /* * Change the definition of the slot identified by the specified name. + * + * Altering the two_phase property of a slot requires caution on the + * client-side. Enabling it at any random point during decoding has the + * risk that transactions prepared before this change may be skipped by + * the decoder, leading to missing prepare records on the client. So, we + * enable it for subscription related slots only once the initial tablesync + * is finished. See comments atop worker.c. Disabling it is safe only when + * there are no pending prepared transaction, otherwise, the changes of + * already prepared transactions can be replicated again along with their + * corresponding commit leading to duplicate data or errors. */ void ReplicationSlotAlter(const char *name, const bool *failover, -- 2.43.0
