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

Hi Vignesh.

Here are some review comments for the patch v20241211-0005.

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

Section "29.6.1. Sequence Definition Mismatches"

1.
+   <warning>
+    <para>
+     If there are differences in sequence definitions between the publisher and
+     subscriber, a WARNING is logged.
+    </para>
+   </warning>

Maybe this should say *when* this happens.

SUGGESTION
During sequence synchronization, the sequence definitions of the
publisher and the subscriber are compared. A WARNING is logged if any
differences are detected.

~~~

Section "29.6.3. Examples"

2.
Should the Examples section also have an example of ALTER SUBSCRIPTION
... REFRESH PUBLICATION to demonstrate (like in the TAP tests) that if
the sequences are already known, then those are not synchronised?

~~~

Section "29.8. Restrictions"

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, but the sequence itself would still show the start value on
+     the subscriber.  If the subscriber is used as a read-only database, then
+     this should typically not be a problem.  If, however, some kind of
+     switchover or failover to the subscriber database is intended, then the
+     sequences would need to be updated to the latest values, either
by executing
+     <link linkend="sql-altersubscription-params-refresh-publication-sequences">
+     <command>ALTER SUBSCRIPTION ... REFRESH PUBLICATION
SEQUENCES</command></link>
+     or by copying the current data from the publisher (perhaps using
+     <command>pg_dump</command>) or by determining a sufficiently high value
+     from the tables themselves.

I don't know if you need to mention it, or maybe it is too obvious,
but the suggestion here to use "ALTER SUBSCRIPTION ... REFRESH
PUBLICATION SEQUENCES" assumed you've already arranged for the
PUBLICATION to be publishing sequences before this.

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

4.
          <para>
-          Specifies whether to copy pre-existing data in the publications
-          that are being subscribed to when the replication starts.
-          The default is <literal>true</literal>.
+          Specifies whether to copy pre-existing data for tables and
synchronize
+          sequences in the publications that are being subscribed to
when the replication
+          starts. The default is <literal>true</literal>.
          </para>

This is talking also about "synchronize sequences" when the
replication starts, but it is a bit confusing. IIUC, the .. REFRESH
PUBLICATION only synchronizes *newly added* sequences anyway, so does
it mean even that will not happen if copy_data=false?

I think this option needs more clarification on how it interacts with
sequences. Also, I don't recall seeing any test for sequences and
copy_data in the patch 0004 TAP tests, so maybe something needs to be
added there too.

~~~

5.
+     <para>
+      See <xref linkend="sequences-out-of-sync"/> for recommendations on how
+      to identify sequences and handle out-of-sync sequences.
+     </para>

/on how to identify sequences and handle out-of-sync sequences./on how
to identify and handle out-of-sync sequences./

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