Hi, Thank you for the patch. I tried v2 and it looks good to me.
What I did: - Applied v2 to current master (ef1b740441c). It applies cleanly (git apply --check) and builds without warnings (--enable-cassert --enable-tap-tests). - t/ https://www.google.com/url?q=http://040_standby_failover_slots_sync.pl&source=gmail&ust=1794597551024000&sa=E passes with the patch (39 tests), and I ran it 5 more times with the same result. - make check passes (243 tests). The whole src/test/recovery TAP suite passes with the patch (59 files, 691 tests; 7 files that need injection points were skipped, as my build has none). - I applied only the t/040 hunk to unpatched master. The new test fails there: the poll for "slotsync_skip_count >= 2 AND temporary AND slotsync_skip_reason = 'no_consistent_snapshot'" times out and returns false (test 39 of 39). So the new test does catch the problem. With the full patch it passes. As expected, the failure comes as a timeout and not as a fast failure. The new "else if (slot->data.persistency == RS_TEMPORARY)" branch looks right to me. I have not tested the back branches, a meson build, pg_sync_replication_slots() with a stopped primary slot, or the original assertion failure scenario by hand, so I can not say anything about those. Thanks again, Joao Rito On Wed, Nov 11, 2026 12:13 PM, Zhijie Hou <[email protected]> wrote: > Hi, > > On Wed, Nov11, 2026 at 01:04 AM Bertrand Drouvot wrote: > > > On Tue, Nov 10, 2026 at 05:17:57PM +0800, Zhijie Hou wrote: > > > I agree with your analysis, and I think we can fix it by preserving > > > SS_SKIP_NO_CONSISTENT_SNAPSHOT in update_local_synced_slot() when > > > the remote slot has not advanced since a previous cycle failed to > > > build a consistent snapshot. > > > > > > Attached is a patch for the fix. I also tried to simplify and > > > stabilize the test and merged it into > > > 040_standby_failover_slots_sync.pl in case we want to include a > > > test. > > > > Thanks for the patch! > > > > + else if (slot->slotsync_skip_reason == > SS_SKIP_NO_CONSISTENT_SNAPSHOT) > > > > Could slotsync_skip_reason be SS_SKIP_WAL_OR_ROWS_REMOVED here even if > > the slot is still inconsistent? This could happen if the local > > catalog_xmin gets ahead and the remote one later catches up? > > Thanks for catching this. I confirmed that this can happen. > > Since any temporary slot that reaches a consistent snapshot is persisted > immediately in the same cycle, we can check "else if > (slot->data.persistency == RS_TEMPORARY)" here instead. Attached is the > v2 patch with this change. > > > -- > Best Regards, > Zhijie Hou
