Re: Logical Replication of sequences

On Thu, 28 Aug 2025 at 17:08, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Vignesh,
>
> Thanks for updating the patch. Below are my comments.
>
> 01.
> ```
>                 /* Relation is either a sequence or a table */
>                 relkind = get_rel_relkind(subrel->srrelid);
>                 if (relkind != RELKIND_SEQUENCE)
>                         continue;
> ```
>
> Can you update the comment to "Skip if the relation is not a sequence"?
> The current one seems not related with what we do.

Modified

> 02.
> ```
>         appendStringInfo(&app_name, "%s_%s", MySubscription->name, "sequencesync worker");
> ```
>
> I'm wondering what is the good application_name. One idea is to follow the
> tablesync worker: "pg_%u_sequence_sync_%u_%llu", another one is to use the same
> name as bgwoker: "logical replication sequencesync worker for subscription %u".
> Your name is also valid but it looks quite different with other processes.
> Do others have any opinions?

Modified

> 03.
> ```
> test_pub=# SELECT NEXTVAL('s1');
> ```
> I feel no need to be capital.

Modified

> 04.
> ```
>    <link linkend="sql-createsubscription"><command>CREATE SUBSCRIPTION</command></link> or
>    <link linkend="sql-altersubscription-params-refresh-publication">
>    <command>ALTER SUBSCRIPTION ... REFRESH PUBLICATION</command></link> or
>    <link linkend="sql-altersubscription-params-refresh-publication-sequences">
>    <command>ALTER SUBSCRIPTION ... REFRESH PUBLICATION SEQUENCES</command></link>.
> ```
>
> IIUC, we can use "A, B, or C" style.

Modified

> 05.
> ```
> +        In logical replication, this parameter also limits how quickly a
> +        failing replication apply worker or table synchronization worker or
> +        sequence synchronization worker will be respawned.
> ```
>
> Same as above.

Modified

> 06.
> ```
>       <para>
>        State codes for sequences:
>        <literal>i</literal> = initialize,
>        <literal>r</literal> = ready
>       </para></entry>
> ```
>
> Now the attribute can be "d".

Modified

> 07.
>
> I feel we should add notes for subscription options. e.g., binary option
> is no-op for sequence sync.

Modified

> 08.
> ```
>         /* Get the list of tables and sequences from the publisher. */
>         if (server_version >= 190000)
>         {
> ```
>
> Not sure which is better, but I considered a way to append the string atop v16.
> Please see attached. How do you feel?

Modified

> 09.
> fetch_relation_list() still has some "tables", which should be "relations".

Modified

> 10.
> ```
>         if (sequencesync_worker)
>         {
>                 /* Now safe to release the LWLock */
>                 LWLockRelease(LogicalRepWorkerLock);
>                 return;
>         }
> ```
>
> Not sure the comment is needed because the lock is acquired just above. Instead
> we can describe that sequence sync worker exists up to one per a subscription.

Removed this comment. I felt no need to add any additional comments here.

> 11.
> Currently copy_sequences() does not check privilege within the function, it is
> done in SetSequence(). Basically it works well, but if the user does not have
> enough privilege, for one of the sequence in the batch, ERROR would be
> raised - all sequences cannot be synched. How about detecting it in the loop
> and skip synchronizing? This allows other sequences to be synced.

Currently we have missing sequences and mismatched sequences being
printed at the end of the batch. We print the skip sequence because it
is dropped concurrently or altered concurrently in the loop. Should we
include this also in the loop itself? Based on it, I will change in
next version.

> 12.
> ```
>         /*
>          * This is a clean exit of the sequencesync worker; reset the
>          * last_seqsync_start_time.
>          */
>         logicalrep_reset_seqsync_start_time();
> ```
>
> ISTM the function can be used both sequence and table sync worker.

It is not required for table sync worker, for table sync worker it is
maintained in last_start_times hash table.

> 13.
> ```
>                 /* Fetch tables and sequences that are in non-ready state. */
>                 rstates = GetSubscriptionRelations(MySubscription->oid, true, true,
>                                                                                    true);
> ```
>
> IIUC no need to check sequences if has_pending_sequences is NULL.

I feel it is required, this is required to determine if a sequence
sync worker should be started or not.

Thanks for the comments, the attached patch has the changes for the same.

Regards,
Vignesh