Few comments: 1) + /* Check if any new sequences need syncing */ + ProcessSequencesForSync(); +
Shall we name it 'maybe_start_seqsync_worker()' (or something better?) and move it immediately after 'maybe_advance_nonremovable_xid()'. Thoughts? This is because we do not process sequences here, we just check if the subscription includes sequences and start worker. 2) We can also change the comment atop ProcessSequencesForSync to mention that. Currently it says: /* * Apply worker determines if sequence synchronization is needed. * * Start a sequencesync worker if one is not already running. Now, we shall change it to: Apply worker determines whether a sequence sync worker is needed. Check if the subscription includes sequences and start a sequencesync worker if one is not already running. 3) I noticed that copy_sequences is invoked twice per sync cycle—once for sequences in the INIT state, and once for sequences in the READY state to detect drift. This means we ping the primary twice during each sync cycle. We should consider combining this logic into a single copy_sequences call. Since we already have the state information (INIT, READY, etc.) for each local sequence, copy_sequences can internally check the current state and decide whether to transition a sequence to READY based on its previous state. This approach would allow us to fetch all required information from the primary in a single call. I also think that we do not even need to mention the states here and we can give a single message instead of 2: DEBUG: logical replication sequence synchronization for subscription "sub1" - for sequences in INIT state batch #1 = 5 attempted, 5 succeeded, 0 mismatched, 0 insufficient permission, 0 missing from publisher, 0 skipped, 0 no drift DEBUG: logical replication sequence synchronization for subscription "sub1" - for sequences in READY state batch #1 = 5 attempted, 0 succeeded, 0 mismatched, 0 insufficient permission, 0 missing from publisher, 0 skipped, 5 no drift 4) FetchRelationStates(): + List *seq_states; It would be better to initialize it with NIL. thanks Shveta
