>
> Thanks for the comments, these are handled in the attached v20250516
> version patch.
>
Thanks for the patches. Here are my review comments -
Patch-0004: src/backend/replication/logical/sequencesync.c
The sequence count logic using curr_seq in copy_sequences() seems buggy.
Currently, curr_seq is incremented based on the number of tuples
received from the publisher inside the inner while loop.
This means it's counting the number of sequences returned by the
publisher, not the number of sequences processed locally. This can
lead to two issues:
1) Repeated syncing of sequences:
If some sequences are missing on the publisher, curr_seq will reflect
fewer items than expected, and subsequent batches may reprocess
already-synced sequences. Because next batch will use curr_seq to get
values from the list as -
seqinfo = (LogicalRepSequenceInfo *)
lfirst(list_nth_cell(remotesequences, curr_seq + i));
Example:
For 110 sequences(s1 to s110), if 5 (s1 to s5) are missing on the
publisher in the first batch, curr_seq = 95. In the next cycle, we
resync s95 to s99.
~~~~
2) Risk of sequencesync worker getting stuck in infinite loop
Consider a case where remotesequences has 10 sequences (s1–s10) need
syncing, and concurrently s9, s10 are deleted on the publisher.
Cycle 1:
Publisher returns s1–s8. So curr_seq = 8.
Cycle 2:
Publisher query returns zero rows (as s9, s10 no longer exist).
curr_seq stays at 8 and never advances.
This causes the while (curr_seq < total_seq) loop to run forever.
~~~~
I think curr_seq should be incremented by batch_seq_count just
outside the inner while loop.
--
Thanks,
Nisha
