Re: Logical Replication of sequences

On Fri, 20 Dec 2024 at 08:05, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Vignesh.
>
> Here are some review comments for the patch v20241211-0005.
>
> ======
>
> 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?

I felt it is not required, let's not add too many examples.

> 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.

I felt it is obvious, it need not be mentioned.

> ======
> 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.

This case is similar to how tables are handled, that is add the table
in subscription_rel and mark it as ready without updating the data.
Since tables and sequences behave the same way I have kept the
documentation the same. I have added a test for this in the 0004 TAP
test.

The rest of the comments are fixed and the changes for the same are
available in the v202412125 version patch at [1].
[1] - https://www.postgresql.org/message-id/CALDaNm0PbSAQvs34D%2BJ63SgmRUzDQHZ1W4aeW_An9pR_tXJnRA%40mail.gmail.com

Regards,
Vignesh