Re: Logical Replication of sequences - Mailing list pgsql-hackers

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