Re: Logical Replication of sequences

On Fri, 20 Dec 2024 at 08:05, Peter Smith <smithpb2250@gmail.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