Re: Logical Replication of sequences

Hi Vignesh.

Some review comments for v20250426-0005.

======
doc/src/sgml/catalogs.sgml

1.
<para>
- State code:
+ State code for tables:
<literal>i</literal> = initialize,
<literal>d</literal> = data is being copied,
<literal>f</literal> = finished table copy,
<literal>s</literal> = synchronized,
<literal>r</literal> = ready (normal replication)
+ </para>
+ <para>
+ State code for sequences:
+ <literal>i</literal> = initialize,
+ <literal>r</literal> = ready
</para></entry>

1a.
There should be an introductory sentence to say what this field is.
e.g. "State code for the table or sequence."

~

1b.
/State code for tables/State codes for tables/

~

1c.
/State code for sequences/State codes for sequences/

======
doc/src/sgml/logical-replication.sgml

2.
+ or <literal>FOR ALL SEQUENCES</literal>. Unlike tables, sequences
allow users
+ to synchronize their current state at any given time. For more information,
+ refer to <xref linkend="logical-replication-sequences"/>.

This is OK, but maybe the "sequences allow users..." is worded
strangely. How about below?

SUGGESTION
Unlike tables, the current state of sequences may be synchronised at any time.

~~~

3.
+ Incremental sequence changes are not replicated. The data in serial or
+ identity columns backed by sequences will of course be replicated as part
+ of the table, the sequences themselves do not replicate ongoing changes.

Seems to be a missing word here

/The data in serial/Although the data in serial/

OR

Just change the punctuation to a semicolon.
/of the table,/of the table;/

======
doc/src/sgml/ref/alter_subscription.sgml

4.
+ <para>
+ Previously subscribed sequences are not re-synchronized. To do that,
+ see <link
linkend="sql-altersubscription-params-refresh-publication-sequences">
+ <command>ALTER SUBSCRIPTION ... REFRESH PUBLICATION
SEQUENCES</command></link>
+ </para>

4a.
Missing period in the last sentence.

~

4b.
AFAIK, when copy_data=false, then not only will *existing* sequences
not be synchronised, but even the *new* sequences will not be
synchronised. Effectively, when copy_data = false, then nothing at all
happens for sequences as far as what the user sees, right?

Experiment:

test_pub=# create publication pub1 for all sequences;
CREATE PUBLICATION

test_sub=# create sequence s1;
CREATE SEQUENCE
NOTICE: created replication slot "sub1" on publisher
CREATE SUBSCRIPTION

test_pub=# create sequence s1;
CREATE SEQUENCE
test_pub=# select * from nextval('s1');
nextval
---------
1
(1 row)

test_pub=# select * from nextval('s1');
nextval
---------
2
(1 row)

test_pub=# select * from nextval('s1');
nextval
---------
3
(1 row)

test_sub=# alter subscription sub1 refresh publication with (copy_data=false);
ALTER SUBSCRIPTION

test_sub=# select * from s1;
last_value | log_cnt | is_called
------------+---------+-----------
1 | 0 | f
(1 row)

So, subscriber side s1 is unaffected.

Maybe it is not worth the effort, but doesn't this mean that you could
optimise the AlterSubscription_refresh() logic to completely skip all
processing for sequences when copy_data=false. e.g. what's the point
of gathering publisher sequence lists and setting INIT states for
them, etc, when it won't synchronise anything because copy_data=false?
Everything will be synchronised later anyway when the user does
REFRESH PUBLICATION SEQUENCES.

======
Kind Regards,
Peter Smith.
Fujitsu Australia