Minor patch; missing comment update in worker.c
Dear hackers,
(Adding Amit in CC because he was an original committer)
While reading codes in master, I found the below comment in worker.c.:
```
* We don't allow to toggle two_phase option of a subscription because it can
* lead to an inconsistent replica. Consider, initially, it was on and we have
* received some prepare then we turn it off, now at commit time the server
* will send the entire transaction data along with the commit. With some more
* analysis, we can allow changing this option from off to on but not sure if
* that alone would be useful.
```
But this is not correct anymore, 1462aad2 allows to alter two_phase option.
I was an original author, but I did oversight.
I feel it can be fixed by referring the commit message, attached patch fixed like
that. How do you feel?
Best regards,
Hayato Kuroda
FUJITSU LIMITED
Attachments:
0001-Fix-missing-comment-update-in-1462aad2.patchapplication/octet-stream; name=0001-Fix-missing-comment-update-in-1462aad2.patchDownload
From 8144245b11cbf5d4610f524ab0c2c2e7be0ff248 Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Mon, 23 Jun 2025 11:45:36 +0900
Subject: [PATCH] Fix missing comment update in 1462aad2
---
src/backend/replication/logical/worker.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index a23262957ac..adbabb9b7d9 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -109,12 +109,11 @@
* If ever a user needs to be aware of the tri-state value, they can fetch it
* from the pg_subscription catalog (see column subtwophasestate).
*
- * We don't allow to toggle two_phase option of a subscription because it can
- * lead to an inconsistent replica. Consider, initially, it was on and we have
- * received some prepare then we turn it off, now at commit time the server
- * will send the entire transaction data along with the commit. With some more
- * analysis, we can allow changing this option from off to on but not sure if
- * that alone would be useful.
+ * We don't allow to toggle two_phase option of a subscription when there are
+ * no pending prepared transactions corresponding to the subscription.
+ * Otherwise, the changes of already prepared transactions can be replicated
+ * again along with their corresponding commit leading to duplicate data or
+ * errors.
*
* Finally, to avoid problems mentioned in previous paragraphs from any
* subsequent (not READY) tablesyncs (need to toggle two_phase option from 'on'
--
2.47.1
On Mon, Jun 23, 2025 at 8:22 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
But this is not correct anymore, 1462aad2 allows to alter two_phase option.
I was an original author, but I did oversight.I feel it can be fixed by referring the commit message, attached patch fixed like
that. How do you feel?
Thanks for the report and patch. I'll look into it.
--
With Regards,
Amit Kapila.
On Mon, Jun 23, 2025 at 8:56 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Mon, Jun 23, 2025 at 8:22 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:But this is not correct anymore, 1462aad2 allows to alter two_phase option.
I was an original author, but I did oversight.I feel it can be fixed by referring the commit message, attached patch fixed like
that. How do you feel?Thanks for the report and patch. I'll look into it.
We can now completely remove that comment, as the necessary details
are already present in subscriptioncmds.c. I have done that and pushed
the patch. Thanks.
--
With Regards,
Amit Kapila.