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

On Wed, Aug 7, 2024 at 1:45 PM vignesh C <vignesh21@gmail.com> wrote:
>
>
> The remaining comments have been addressed, and the changes are
> included in the attached v20240807 version patch.

Thanks for addressing the comment. Please find few comments for v20240807 :

patch002:
1)
create_publication.sgml:

--I think it will be good to add another example for both tables and sequences:
CREATE PUBLICATION all_sequences FOR ALL TABLES, SEQUENCES;
I was trying FOR ALL TABLES, FOR ALL SEQUENCES; but I think it is not
the correct way, so good to have the correct way mentioned in one
example.

patch003:
2)

 * The page_lsn allows the user to determine if the sequence has been updated
 * since the last synchronization with the subscriber. This is done by
 * comparing the current page_lsn with the value stored in pg_subscription_rel
 * from the last synchronization.
 */
Datum
pg_sequence_state(PG_FUNCTION_ARGS)

--This information is still incomplete. Maybe we should mention the
other attribute name as well which helps to determine this.

3)
Shall process_syncing_sequences_for_apply() be moved to sequencesync.c

4)
Would it be better to give a single warning for all unequal sequences
(comma separated list of sequenec names?)

postgres=# create subscription sub1 connection '....' publication pub1;
WARNING:  Sequence parameter in remote and local is not same for "public.myseq2"
HINT:  Alter/Re-create the sequence using the same parameter as in remote.
WARNING:  Sequence parameter in remote and local is not same for "public.myseq0"
HINT:  Alter/Re-create the sequence using the same parameter as in remote.
WARNING:  Sequence parameter in remote and local is not same for "public.myseq4"
HINT:  Alter/Re-create the sequence using the same parameter as in remote.


5)
IIUC, sequencesync_failure_time is changed by multiple processes.
Seq-sync worker sets it before exiting on failure, while apply worker
resets it. Also, the applied worker reads it at a few places. Shall it
be accessed using LogicalRepWorkerLock?

6)
process_syncing_sequences_for_apply():

--I feel MyLogicalRepWorker->sequencesync_failure_time should be reset
to 0 after we are sure that logicalrep_worker_launch() has launched
the worker without any error. But not sure what could be the clean way
to do it? If we move it after logicalrep_worker_launch() call, there
are chances that seq-sync worker has started and failed already and
has set this failure time which will then be mistakenly reset by apply
worker. Also moving it inside logicalrep_worker_launch() does not seem
a good way.

7)
sequencesync.c
PostgreSQL logical replication: initial sequence synchronization

--Since it is called by REFRESH also. So shall we remove 'initial'?

8)
/*
* Process any tables that are being synchronized in parallel and
* any newly added relations.
*/
process_syncing_relations(last_received);

--I did not understand the comment very well. Why are we using 2
separate words 'tables' and 'relations'? I feel we should have
mentioned sequences too in the comment.


9)
logical-replication.sgml: Sequence data is not replicated.

--I feel we should rephrase this line now to indicate that it could be
replicated by the new options.

thanks
Shveta