Re: Logical Replication of sequences

On Wed, Jul 30, 2025 at 11:16 AM 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.
>
> 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.
>
> 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.
>
> 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.
>
> 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).
>
> 5)
> + * Reset the last_start_time of the sequencesync worker in the subscription's
> + * apply worker.
>
> last_start_time-->last_seqsync_start_time
>
> 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.
>

7)
I am trying to understand the flow of check_and_launch_sync_worker().
We acquire the lock(LogicalRepWorkerLock) in the caller and release it
here. This does not look appropriate. I guess, both
logicalrep_worker_find() and logicalrep_sync_worker_count() need lock
to be held, that is why we have done this.  I see that
logicalrep_worker_launch() (invoked by check_and_launch_sync_worker())
also does logicalrep_sync_worker_count() and also tries
garbage-collection once. Shouldn't that suffice? Or is there any
reason to call logicalrep_sync_worker_count() in
check_and_launch_sync_worker() additionally? If
logicalrep_sync_worker_count() is not needed to be called from
check_and_launch_sync_worker(), the LOCK problem is sorted.

thanks
Shveta