Wrong comment for ReplicationSlotCreate
Hi,
I noticed that the comment for ReplicationSlotCreate function contains this
description for the "two_phase" option :
* 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.
But commit [1]1462aad2e4474ab61174f8ab00992cd3d6d57c7b introduced the ability to alter the "two_phase" option for the
replication slot. Thus, I guess that the comment mentioned above is
outdated and we should change it.
[1]: 1462aad2e4474ab61174f8ab00992cd3d6d57c7b
--
Best regards,
Daniil Davydov
Attachments:
0001-Fix-comment-for-ReplicationSlotCreate.patchtext/x-patch; charset=US-ASCII; name=0001-Fix-comment-for-ReplicationSlotCreate.patchDownload
From 9545671c48ee75ee7f9eed26af0902fcd3114573 Mon Sep 17 00:00:00 2001
From: Daniil Davidov <d.davydov@postgrespro.ru>
Date: Mon, 29 Dec 2025 20:27:11 +0700
Subject: [PATCH] Fix comment for ReplicationSlotCreate
---
src/backend/replication/slot.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 58c41d45516..310dcfcbd5f 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.
--
2.43.0
On Dec 29, 2025, at 21:39, Daniil Davydov <3danissimo@gmail.com> wrote:
Hi,
I noticed that the comment for ReplicationSlotCreate function contains this
description for the "two_phase" option :* 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.But commit [1] introduced the ability to alter the "two_phase" option for the
replication slot. Thus, I guess that the comment mentioned above is
outdated and we should change it.[1] 1462aad2e4474ab61174f8ab00992cd3d6d57c7b
--
Best regards,
Daniil Davydov
<0001-Fix-comment-for-ReplicationSlotCreate.patch>
The old comment claimed “We allow this option to be enabled only at the slot creation time” that is no longer true after commit 1462aad2e4474ab61174f8ab00992cd3d6d57c7b, so yes, the old comment need updating.
But I think the updated version is too simple. It loses the information that enabling two_phase later can result in missing PREPARE.
So, I would suggest something like:
```
* two_phase: If enabled, allows decoding of prepared transactions.
* Note that enabling this option after decoding has already advanced
* may result in missing PREPARE records for transactions that were
* prepared before the option was enabled.
```
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Hi,
On Tue, Dec 30, 2025 at 4:18 PM Chao Li <li.evan.chao@gmail.com> wrote:
But I think the updated version is too simple. It loses the information that enabling two_phase later can result in missing PREPARE.
So, I would suggest something like:
```
* two_phase: If enabled, allows decoding of prepared transactions.
* Note that enabling this option after decoding has already advanced
* may result in missing PREPARE records for transactions that were
* prepared before the option was enabled.```
Thanks for the review!
As far as I understand, if the publisher prepares a transaction and
then subscriber
tries to create a subscription, walsender will wait until the prepared
transaction is
finished (during execution of CREATE_REPLICATION_SLOT command).
We can find this logic inside the SnapBuildFindSnapshot function. Thus, we
cannot miss any PREPARE record for the created slot.
Am I missing something?
--
Best regards,
Daniil Davydov
On Dec 30, 2025, at 18:07, Daniil Davydov <3danissimo@gmail.com> wrote:
Hi,
On Tue, Dec 30, 2025 at 4:18 PM Chao Li <li.evan.chao@gmail.com> wrote:
But I think the updated version is too simple. It loses the information that enabling two_phase later can result in missing PREPARE.
So, I would suggest something like:
```
* two_phase: If enabled, allows decoding of prepared transactions.
* Note that enabling this option after decoding has already advanced
* may result in missing PREPARE records for transactions that were
* prepared before the option was enabled.```
Thanks for the review!
As far as I understand, if the publisher prepares a transaction and
then subscriber
tries to create a subscription, walsender will wait until the prepared
transaction is
finished (during execution of CREATE_REPLICATION_SLOT command).
We can find this logic inside the SnapBuildFindSnapshot function. Thus, we
cannot miss any PREPARE record for the created slot.Am I missing something?
--
Best regards,
Daniil Davydov
You’re right that during CREATE_REPLICATION_SLOT, SnapBuildFindSnapshot() ensures we don’t miss PREPARE records for prepared transactions that exist at creation time.
1462aad2e introduced support for altering logical replication slot options, including two_phase, after the slot has been created. The commit comment says:
```
Changing the 'two_phase' option for a subscription from 'true' to 'false'
is permitted only when there are no pending prepared transactions
corresponding to that subscription. Otherwise, the changes of already
prepared transactions can be replicated again along with their corresponding
commit leading to duplicate data or errors.
```
true->false is not allowed, however false->true is permitted. So, I think the old comment is still possible today:
```
* Note that enabling this option after decoding has already advanced
* may result in missing PREPARE records for transactions that were
* prepared before the option was enabled.
```
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Hi,
On Wed, Dec 31, 2025 at 9:32 AM Chao Li <li.evan.chao@gmail.com> wrote:
You’re right that during CREATE_REPLICATION_SLOT, SnapBuildFindSnapshot() ensures we don’t miss PREPARE records for prepared transactions that exist at creation time.
1462aad2e introduced support for altering logical replication slot options, including two_phase, after the slot has been created. The commit comment says:
```
Changing the 'two_phase' option for a subscription from 'true' to 'false'
is permitted only when there are no pending prepared transactions
corresponding to that subscription. Otherwise, the changes of already
prepared transactions can be replicated again along with their corresponding
commit leading to duplicate data or errors.
```true->false is not allowed, however false->true is permitted. So, I think the old comment is still possible today:
```
* Note that enabling this option after decoding has already advanced
* may result in missing PREPARE records for transactions that were
* prepared before the option was enabled.
```
Hm, I still can't understand why the comment that you provided is correct.
How can we "miss PREPARE records" if slot creation requires all prepared
transactions to finish? The commit message says about risks during the
change of the parameter "on the fly". But we are dealing with slot creation.
--
Best regards,
Daniil Davydov
On Jan 2, 2026, at 00:31, Daniil Davydov <3danissimo@gmail.com> wrote:
Hi,
On Wed, Dec 31, 2025 at 9:32 AM Chao Li <li.evan.chao@gmail.com> wrote:
You’re right that during CREATE_REPLICATION_SLOT, SnapBuildFindSnapshot() ensures we don’t miss PREPARE records for prepared transactions that exist at creation time.
1462aad2e introduced support for altering logical replication slot options, including two_phase, after the slot has been created. The commit comment says:
```
Changing the 'two_phase' option for a subscription from 'true' to 'false'
is permitted only when there are no pending prepared transactions
corresponding to that subscription. Otherwise, the changes of already
prepared transactions can be replicated again along with their corresponding
commit leading to duplicate data or errors.
```true->false is not allowed, however false->true is permitted. So, I think the old comment is still possible today:
```
* Note that enabling this option after decoding has already advanced
* may result in missing PREPARE records for transactions that were
* prepared before the option was enabled.
```Hm, I still can't understand why the comment that you provided is correct.
How can we "miss PREPARE records" if slot creation requires all prepared
transactions to finish? The commit message says about risks during the
change of the parameter "on the fly". But we are dealing with slot creation.--
Best regards,
Daniil Davydov
No problem, maybe I am wrong. Then please ignore my comment and wait for other review comments.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Dear Daniil, Chao,
I was the main author of 1462aad2. It is enough to remove outdated comments atop
the definition. In other words, your patch looks good to me.
If needed, we can also notify developers that the two-phase option should not be
altered while decoding WAL records. In logical replication, we ensure that the
subscription is disabled and there are no apply workers. However, I don't think
such comments can be atop the ReplicationSlotCreate(). Maybe around
ReplicationSlotAlter(), but it may be out of scope of the initial motivation.
By the way, the comment may have been broken since a8fd13. Even when the
subscription was defined with two_phase=on, the backend creates the slot with
two_phase = off. The configuration is changed after the tablesync is done.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
On Thu, Jan 1, 2026 at 10:01 PM Daniil Davydov <3danissimo@gmail.com> wrote:
On Wed, Dec 31, 2025 at 9:32 AM Chao Li <li.evan.chao@gmail.com> wrote:
You’re right that during CREATE_REPLICATION_SLOT, SnapBuildFindSnapshot() ensures we don’t miss PREPARE records for prepared transactions that exist at creation time.
1462aad2e introduced support for altering logical replication slot options, including two_phase, after the slot has been created. The commit comment says:
```
Changing the 'two_phase' option for a subscription from 'true' to 'false'
is permitted only when there are no pending prepared transactions
corresponding to that subscription. Otherwise, the changes of already
prepared transactions can be replicated again along with their corresponding
commit leading to duplicate data or errors.
```true->false is not allowed, however false->true is permitted. So, I think the old comment is still possible today:
```
* Note that enabling this option after decoding has already advanced
* may result in missing PREPARE records for transactions that were
* prepared before the option was enabled.
```Hm, I still can't understand why the comment that you provided is correct.
How can we "miss PREPARE records" if slot creation requires all prepared
transactions to finish? The commit message says about risks during the
change of the parameter "on the fly". But we are dealing with slot creation.
The point is about changing the two_phase property of slot after
slot_creation which we carefully change for slots related to
subscription. So, if we don't take necessary precautions then enabling
it on the fly could lead to the problem Chao is worried about.
--
With Regards,
Amit Kapila.
On Mon, Jan 5, 2026 at 9:46 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
Dear Daniil, Chao,
I was the main author of 1462aad2. It is enough to remove outdated comments atop
the definition. In other words, your patch looks good to me.If needed, we can also notify developers that the two-phase option should not be
altered while decoding WAL records. In logical replication, we ensure that the
subscription is disabled and there are no apply workers. However, I don't think
such comments can be atop the ReplicationSlotCreate(). Maybe around
ReplicationSlotAlter(), but it may be out of scope of the initial motivation.
I think it is better if we add some comments atop
ReplicationSlotAlter() as you are suggesting. What do you think of the
attached?
--
With Regards,
Amit Kapila.
Attachments:
v1_improve_alter_slot_comments.patchapplication/octet-stream; name=v1_improve_alter_slot_comments.patchDownload
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 58c41d45516..641245bc809 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
+ * clinet-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,
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.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
On Jan 5, 2026, at 13:51, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Mon, Jan 5, 2026 at 9:46 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:Dear Daniil, Chao,
I was the main author of 1462aad2. It is enough to remove outdated comments atop
the definition. In other words, your patch looks good to me.If needed, we can also notify developers that the two-phase option should not be
altered while decoding WAL records. In logical replication, we ensure that the
subscription is disabled and there are no apply workers. However, I don't think
such comments can be atop the ReplicationSlotCreate(). Maybe around
ReplicationSlotAlter(), but it may be out of scope of the initial motivation.I think it is better if we add some comments atop
ReplicationSlotAlter() as you are suggesting. What do you think of the
attached?--
With Regards,
Amit Kapila.
<v1_improve_alter_slot_comments.patch>
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.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
slot_typo.diffapplication/octet-stream; name=slot_typo.diff; x-unix-mode=0644Download
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 75d26fa61ea..ce41441c581 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -635,7 +635,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)
{
Hi,
On Mon, Jan 5, 2026 at 1:25 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> 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 <li.evan.chao@gmail.com> 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
Attachments:
v2-0002-Improve-comments-for-ReplicationSlotAcquire.patchtext/x-patch; charset=US-ASCII; name=v2-0002-Improve-comments-for-ReplicationSlotAcquire.patchDownload
From ea061b3f16a9449fd49d4188dde9620d7992ebbf Mon Sep 17 00:00:00 2001
From: Daniil Davidov <d.davydov@postgrespro.ru>
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
v2-0001-Improve-comments-for-ReplicationSlotAlter.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Improve-comments-for-ReplicationSlotAlter.patchDownload
From e955164e14cff2f67727a0144056610426dcc825 Mon Sep 17 00:00:00 2001
From: Daniil Davidov <d.davydov@postgrespro.ru>
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
On Mon, Jan 5, 2026 at 4:01 PM Daniil Davydov <3danissimo@gmail.com> wrote:
Good catch!
I am not sure that we should put both fixes together, so I'll put your fix in a
separate patch.
It is okay to fix the typo patch in HEAD but the other patch related
to fixing the outdated comment needs to be backpatched, so pushed
separately. Thanks!
--
With Regards,
Amit Kapila.