Typo in tablesync comment

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 and

This 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 and

This 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 and

This 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

On Thu, Feb 04, 2021 at 10:50:11AM +1100, Peter Smith wrote:

OK. I attached an updated patch using this new text.

Thanks, applied.
--
Michael