Re: Logical Replication of sequences

On Thu, May 29, 2025 at 8:09 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> These comments are handled in the attached v2025029 version patch.
>

Thanks for the patches. I am still reviewing but please find few comments:

1)
<para>
Only persistent sequences are included in the publication. Temporary
sequences are excluded from the publication.
</para>

We shall mention UNLOGGED sequences as well along with TEMP sequences.

2)
Why do we have GetAllSequencesPublicationRelations() in patch002? It
is used only in patch004. Same thing with is_publishable_class()
change.

3)
process_syncing_tables_for_sync() is renamed to ProcessSyncingTablesForSync()
process_syncing_tables_for_apply() is renamed to ProcessSyncingTablesForApply()
process_syncing_tables() is renamed to SyncProcessRelations()

Why have we named it SyncProcessRelations and not
ProcessSyncingRelations? Is it because we want to start a name with
'Sync' in order to have file name initials? But do not see other files
following it. IMO, ProcessSyncingTables looks more familiar and apt
over SyncProcessRelations. Same with 'SyncFinishWorker'.
FinishSyncWorker instead looks better. Thoughts?

4)
postgres=# CREATE publication pub1 for sequences;
ERROR: invalid publication object list
LINE 1: CREATE publication pub1 for sequences;
^
DETAIL: One of TABLE or TABLES IN SCHEMA must be specified before a
standalone table or schema name.

Do you think we shall mention sequence specific info as well in DETAIL now?

thanks
Shveta