Fixed crashes due to missed slotToKeyInit() and missed expires_cursor reset #13315
Conversation
sundb
added
the
release-notes
tests/unit/memefficiency.tcl
Outdated
| @@ -37,9 +37,10 @@ start_server {tags {"memefficiency external:skip"}} { | |||
| } | |||
|
|
|||
| run_solo {defrag} { | |||
| start_server {tags {"defrag external:skip"} overrides {appendonly yes auto-aof-rewrite-percentage 0 save ""}} { | |||
| proc test_active_defrag {type} { | |||
There was a problem hiding this comment.
so now all the defrag tests are running twice?
isn't that a little excessive?
if we keep it, or find a better one, do we want to reflect that change in unstable?
There was a problem hiding this comment.
but these two are not the same, one is for cluster, and another is for standalone.
There was a problem hiding this comment.
i know.
i'm saying that in the past we had a bunch of tests: [plain, aof, large keys, edge case, eval]
and now we run nearly all of them twice.
i think it's excessive, it's probably enough to just run one or two of them twice, or just add a dedicated test for cluster defrag (which i think we already have in unstable)
There was a problem hiding this comment.
or just add a dedicated test for cluster defrag (which i think we already have in unstable)
i'll do it.
There was a problem hiding this comment.
@sundb so to be sure: with this PR getting merged, the tests in both 7.2 and 7.4 are now able to detect a bug similar to the one being fixed here (which doesn't exist in 7.4)?
There was a problem hiding this comment.
no, 7.4 doesn't covert it unless we add the same test for it.
There was a problem hiding this comment.
ohh, now i actually looked at the test code:
# It repeatedly enables and disables active defragmentation,
# and checks if it crashes, see issue #13307.
well, i suppose it's pointless to test the expires_cursor reset thing, since that code is now simplified.
and the flushdb async bug isn't really related to the defrag, so if we wanted, we could have added another simpler explicit check for it.
truth be told, that emptyDbAsync does have some duplicated logic in it that can someday get out of sync.
but i suppose that's completely unrelated to this PR.
this PR fixes two crashes:
Fix missing slotToKeyInit() when using
flushdb asyncunder cluster mode.[CRASH] Redis 7.2.3 crashed in slotToKeyReplaceEntry #13205
Fix missing expires_cursor reset when stopping active defrag in the middle of defragment.
[CRASH] Redis 7.2.x crashes in activeDefragCycle when activedefrag disabled while running and re-enabled #13307
If we stop active defrag in the middle of defragging db->expires, if
expires_cursoris not reset to 0, the next time we enable active defrag again, defragLaterStep(db, ...) will be entered. However, at this time,dbhas been reset to NULL, which results in crash.The affected code were removed by #11695 and #13058 in usntable, so we just need backport this to 7.2.