> > 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
