RE: Logical Replication of sequences - Mailing list pgsql-hackers

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.

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?

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

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.

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.

06.
```
      <para>
       State codes for sequences:
       <literal>i</literal> = initialize,
       <literal>r</literal> = ready
      </para></entry>
```

Now the attribute can be "d".

07.

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

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?

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

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.

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.

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.

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.

Best regards,
Hayato Kuroda
FUJITSU LIMITED