Hi Ajin, I have reviewed v20 and got a few comments:
> On Oct 30, 2025, at 18:18, Ajin Cherian <[email protected]> wrote: > > <v20-0001-Improve-initial-slot-synchronization-in-pg_sync_.patch> 1 - slotsync.c ``` + if (slot_names) + list_free_deep(slot_names); /* Cleanup the synced temporary slots */ ReplicationSlotCleanup(true); @@ -1762,5 +2026,5 @@ SyncReplicationSlots(WalReceiverConn *wrconn) /* We are done with sync, so reset sync flag */ reset_syncing_flag(); } - PG_END_ENSURE_ERROR_CLEANUP(slotsync_failure_callback, PointerGetDatum(wrconn)); + PG_END_ENSURE_ERROR_CLEANUP(slotsync_failure_callback, PointerGetDatum(&fparams)); ``` I am afraid there is a risk of double memory free. Slot_names has been assigned to fparams.slot_names within the for loop, and it’s freed after the loop. If something gets wrong and slotsync_failure_callback() is called, the function will free fparams.slot_names again. 2 - slotsync.c ``` + /* + * Fetch remote slot info for the given slot_names. If slot_names is NIL, + * fetch all failover-enabled slots. Note that we reuse slot_names from + * the first iteration; re-fetching all failover slots each time could + * cause an endless loop. Instead of reprocessing only the pending slots + * in each iteration, it's better to process all the slots received in + * the first iteration. This ensures that by the time we're done, all + * slots reflect the latest values. + */ + remote_slots = fetch_remote_slots(wrconn, slot_names); + + /* Attempt to synchronize slots */ + some_slot_updated = synchronize_slots(wrconn, remote_slots, + &slot_persistence_pending); + + /* + * If slot_persistence_pending is true, extract slot names + * for future iterations (only needed if we haven't done it yet) + */ + if (slot_names == NIL && slot_persistence_pending) + { + slot_names = extract_slot_names(remote_slots); + + /* Update the failure structure so that it can be freed on error */ + fparams.slot_names = slot_names; + } ``` I am thinking if that could be a problem. As you now extract_slot_names() only in the first iteration, if a slot is dropped, and a new slot comes with the same name, will the new slot be incorrectly synced? Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
