Re: Logical Replication of sequences

On Wed, 30 Jul 2025 at 11:16, shveta malik <shveta.malik@gmail.com> wrote:
>
> On Mon, Jul 28, 2025 at 3:37 PM vignesh C <vignesh21@gmail.com> wrote:
>
> > Thanks for the comments, the attached v20250728 version patch has the
> > changes for the same.
> >
> Thanks for the patches, please find a few comments:
>
> 1)
> WARNING:  WITH clause parameters do not affect sequence synchronization
>
> a)
> How about:
> WITH clause parameters are not applicable to sequence synchronization
> or
> WITH clause parameters are not applicable to sequence synchronization
> and will be ignored.

Modified

> b)
> Should it be NOTICE OR WARNING? I feel NOTICE Is more appropriate as
> it is more of an information than a warning since it has no negative
> consequences.

Modified

> 2)
>  AlterSubscription_refresh(Subscription *sub, bool copy_data,
> -   List *validate_publications)
> +   List *validate_publications, bool refresh_tables,
> +   bool refresh_sequences, bool resync_all_sequences)
>  {
>
> Do we need 3 new arguments? If we notice, 'refresh_sequences' is
> always true in all cases. I feel only the last one should suffice.
> IIUC, this is the state:
>
> When resync_all_sequences is true:
> it indicates it is 'REFRESH PUBLICATION SEQUENCES', that means we have
> to refresh new sequences and resync all sequences.
>
> When resync_all_sequences is false:
> That means it is 'REFRESH PUBLICATION', we have to refresh new tables
> and new sequences alone.
>
> So if the caller pass only 'resync_all_sequences', we should be able
> to drive the rest of the values internally.

Modified

> 3)
>  ALTER SUBSCRIPTION regress_testsub REFRESH PUBLICATION;
> -ERROR:  ALTER SUBSCRIPTION ... REFRESH cannot run inside a transaction block
> +ERROR:  ALTER SUBSCRIPTION ... REFRESH PUBLICATION cannot run inside
> a transaction block
>
> In the same script, we can test REFRESH PUBLICATION SEQUENCES also in
> trancsation block.

Added it

> 4)
> Commit message of patch004 says:
>
> This patch introduce a new command to synchronize the sequences of
> a subscription:
> ALTER SUBSCRIPTION ... REFRESH PUBLICATION SEQUENCES
>
> a)
> introduce --> introduces
>
> b)
> We should also add:
>
> This patch also changes the scope of
> ALTER SUBSCRIPTION ... REFRESH PUBLICATION
> This command now also considers sequences (newly added or dropped ones).

Modified the commit message now to add this information

> 5)
> + * Reset the last_start_time of the sequencesync worker in the subscription's
> + * apply worker.
>
> last_start_time-->last_seqsync_start_time

Modified

> 6)
> alter_subscription.sgml has this:
>         <term><literal>refresh</literal> (<type>boolean</type>)</term>
>         <listitem>
>          <para>
>           When false, the command will not try to refresh table information.
>           <literal>REFRESH PUBLICATION</literal> should then be
> executed separately.
>           The default is <literal>true</literal>.
>          </para>
>         </listitem>
>        </varlistentry>
>
> Shouldn't we mention sequence too here:
> When false, the command will not try to refresh table and sequence information.

Modified

Also, the comment from [1] has been addressed — the lock is now
released from the caller function itself. Previously, the lock was
required to fetch the number of sync workers running, but this has
been refactored to retrieve the count within the caller function
instead.

Thanks for the comments, the attached patch has the changes for the same.
[1] - https://www.postgresql.org/message-id/CAJpy0uBCOmoyc44J46PpHbip0Sovqm99cL%3DAJoAErXG0EN2Duw%40mail.gmail.com

Regards,
Vignesh