Typo in tablesync comment
PSA a trivial patch to correct what seems like a typo in the tablesync comment.
----
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
fix-tablesync-comment-typo-20210202.patchapplication/octet-stream; name=fix-tablesync-comment-typo-20210202.patchDownload
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 863d196..18131cc 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -52,8 +52,8 @@
* CATCHUP -> SYNCDONE -> READY.
*
* The catalog pg_subscription_rel is used to keep information about
- * subscribed tables and their state. Some transient state during data
- * synchronization is kept in shared memory. The states SYNCWAIT and
+ * subscribed tables and their state. Some transient states during data
+ * synchronization are kept in shared memory. The states SYNCWAIT and
* CATCHUP only appear in memory.
*
* Example flows look like this:
On Tue, Feb 02, 2021 at 10:38:31AM +1100, Peter Smith wrote:
PSA a trivial patch to correct what seems like a typo in the tablesync comment.
- * subscribed tables and their state. Some transient state during data
- * synchronization is kept in shared memory. The states SYNCWAIT and
+ * subscribed tables and their state. Some transient states during data
+ * synchronization are kept in shared memory. The states SYNCWAIT andThis stuff refers to SUBREL_STATE_* in pg_subscription_rel.h, and FWIW
I find confusing the term "transient" in this context as a state may
last for a rather long time, depending on the time it takes to
synchronize the relation, no? I am wondering if we could do better
here, say:
"The state tracking the progress of the relation synchronization is
additionally stored in shared memory, with SYNCWAIT and CATCHUP only
appearing in memory."
--
Michael
On Tue, Feb 2, 2021, at 2:19 AM, Michael Paquier wrote:
On Tue, Feb 02, 2021 at 10:38:31AM +1100, Peter Smith wrote:
PSA a trivial patch to correct what seems like a typo in the tablesync comment.
- * subscribed tables and their state. Some transient state during data - * synchronization is kept in shared memory. The states SYNCWAIT and + * subscribed tables and their state. Some transient states during data + * synchronization are kept in shared memory. The states SYNCWAIT andThis stuff refers to SUBREL_STATE_* in pg_subscription_rel.h, and FWIW
I find confusing the term "transient" in this context as a state may
last for a rather long time, depending on the time it takes to
synchronize the relation, no? I am wondering if we could do better
here, say:
"The state tracking the progress of the relation synchronization is
additionally stored in shared memory, with SYNCWAIT and CATCHUP only
appearing in memory."
WFM.
--
Euler Taveira
EDB https://www.enterprisedb.com/
On Tue, Feb 2, 2021 at 10:49 AM Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Feb 02, 2021 at 10:38:31AM +1100, Peter Smith wrote:
PSA a trivial patch to correct what seems like a typo in the tablesync comment.
- * subscribed tables and their state. Some transient state during data - * synchronization is kept in shared memory. The states SYNCWAIT and + * subscribed tables and their state. Some transient states during data + * synchronization are kept in shared memory. The states SYNCWAIT andThis stuff refers to SUBREL_STATE_* in pg_subscription_rel.h, and FWIW
I find confusing the term "transient" in this context as a state may
last for a rather long time, depending on the time it takes to
synchronize the relation, no?
These in-memory states are used after the initial copy is done. So,
these are just for the time the tablesync worker is synced-up with
apply worker. In some cases, they could be for a longer period of time
when apply worker is quite ahead of tablesync worker then we will be
in the CATCHUP state for a long time but SYNCWAIT will still be for a
shorter period of time.
I am wondering if we could do better
here, say:
"The state tracking the progress of the relation synchronization is
additionally stored in shared memory, with SYNCWAIT and CATCHUP only
appearing in memory."
I don't mind changing to your proposed text but I think the current
wording is also okay and seems clear to me.
--
With Regards,
Amit Kapila.
On Tue, Feb 02, 2021 at 07:23:37PM +0530, Amit Kapila wrote:
I don't mind changing to your proposed text but I think the current
wording is also okay and seems clear to me.
Reading that again, I still find the word "transient" to be misleading
in this context. Any extra opinions?
--
Michael
On Wed, Feb 3, 2021 at 6:13 PM Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Feb 02, 2021 at 07:23:37PM +0530, Amit Kapila wrote:
I don't mind changing to your proposed text but I think the current
wording is also okay and seems clear to me.Reading that again, I still find the word "transient" to be misleading
in this context. Any extra opinions?
OTOH I thought "additionally stored" made it seem like those states
were in the catalog and "additionally" in shared memory.
Maybe better to rewrite it more drastically?
e.g
-----
* The catalog pg_subscription_rel is used to keep information about
* subscribed tables and their state. The catalog holds all states
* except SYNCWAIT and CATCHUP which are only in shared memory.
-----
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Feb 03, 2021 at 06:52:56PM +1100, Peter Smith wrote:
OTOH I thought "additionally stored" made it seem like those states
were in the catalog and "additionally" in shared memory.
Good point.
Maybe better to rewrite it more drastically?
e.g
-----
* The catalog pg_subscription_rel is used to keep information about
* subscribed tables and their state. The catalog holds all states
* except SYNCWAIT and CATCHUP which are only in shared memory.
-----
Fine by me.
--
Michael
On Wed, Feb 3, 2021 at 1:35 PM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Feb 03, 2021 at 06:52:56PM +1100, Peter Smith wrote:
Maybe better to rewrite it more drastically?
e.g
-----
* The catalog pg_subscription_rel is used to keep information about
* subscribed tables and their state. The catalog holds all states
* except SYNCWAIT and CATCHUP which are only in shared memory.
-----Fine by me.
+1.
--
With Regards,
Amit Kapila.
On Wed, Feb 3, 2021 at 11:55 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Wed, Feb 3, 2021 at 1:35 PM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Feb 03, 2021 at 06:52:56PM +1100, Peter Smith wrote:
Maybe better to rewrite it more drastically?
e.g
-----
* The catalog pg_subscription_rel is used to keep information about
* subscribed tables and their state. The catalog holds all states
* except SYNCWAIT and CATCHUP which are only in shared memory.
-----Fine by me.
+1.
OK. I attached an updated patch using this new text.
----
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v2-0001-Fix-tablesync-comment-typo.patchapplication/octet-stream; name=v2-0001-Fix-tablesync-comment-typo.patchDownload
From 6fdcbb189400bd053bdd45a25f93f6f55754b7cc Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Thu, 4 Feb 2021 10:37:24 +1100
Subject: [PATCH v2] Fix tablesync comment typo
---
src/backend/replication/logical/tablesync.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index a18f847..cc7cc0f 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -52,9 +52,8 @@
* CATCHUP -> SYNCDONE -> READY.
*
* The catalog pg_subscription_rel is used to keep information about
- * subscribed tables and their state. Some transient state during data
- * synchronization is kept in shared memory. The states SYNCWAIT and
- * CATCHUP only appear in memory.
+ * subscribed tables and their state. The catalog holds all states
+ * except SYNCWAIT and CATCHUP which are only in shared memory.
*
* Example flows look like this:
* - Apply is in front:
--
1.8.3.1