@zuiderkwast thank you for revamping that stale PR.
i actually no longer remember the status there and why it was stale.
maybe you can quickly remind me, and also list the differences between this one and the old one?

maybe you can quickly remind me, and also list the differences between this one and the old one?

@oranagra The problem: WATCH an already expired key, then active expire or DEL marks the key as dirty, causing EXEC to fail, although there was no actual change in the logical existence of the key since WATCH.

The old PR attempted to delete expired keys on WATCH (expireIfNeeded), making WATCH a command with side effects that need replication.

This PR is based on your idea of storing a bit somewhere, flagging that the keys was already expired when WATCH was called. Then, if the key is deleted or active-expired after WATCH, this doesn't mark the key as dirty if the flag is set.

Problem solved without a need for replicating side-effects of WATCH. We don't care if the key is actually removed from the db dict or not.

Oops... I have already implemented it in my laptop but forgot to push it, it's similar with your codes. I just pushed it and you can compare with the original PR.

BTW, the implements both this PR and mine are incomplete, the touchAllWatchedKeysInDb() also needs check if watched keys are expired/stale, since FLUSHALL/FLUSHDB/SWAPDB would call it, and SWAPDB is irritating.

Oops... I have already implemented it in my laptop but forgot to push it, it's similar with your codes. I just pushed it and you can compare with the original PR.

@soloestoy OK, it seems to have diverged from unstable. Do you want to continue your work or shall we focus on this PR? We can avoid doing parallel work...

BTW, the implements both this PR and mine are incomplete, the touchAllWatchedKeysInDb() also needs check if watched keys are expired/stale, since FLUSHALL/FLUSHDB/SWAPDB would call it, and SWAPDB is irritating.

Good point!

Looking at touchAllWatchedKeysInDb(), I think it may have a bug since it doesn't touch deleted keys. Correct me if I'm wrong.
I'm not sure what's the expected behaviour for the stale keys in touchAllWatchedKeysInDb.

@soloestoy please have a look before i merge this.

@oranagra Please don't merge yet. I have found some errors for swapdb in a not-yet-committed test case "SWAPDB does not touch non-existing key replaced with stale key". It turns out the database dicts are swapped before touchAllKeysInDb are called. In the flushdb command, the dicts are emptied after touchAllKeysInDb is called.

More test cases for SWAPDB found more errors that are now fixed:

1. Expired key replaced with non-existing key (clear expired flag)
2. Non-existing key replaced with expired key (set expired flag)
3. Expired key replaced with expired key (key remains expired)

@oranagra It's unfortunate that touchAllWatchedKeysInDb is called after swapping the dicts when it's called before flushing dics in FLUSHDB. I'll try to change this so touchAllWatchedKeysInDb is called before swapping the dics and see if it breaks other things.

this somehow break the test (timing issue in valgrind and FreeBsd):

https://github.com/enjoy-binbin/redis/runs/5289611534?check_suite_focus=true#step:6:4999
*** [err]: EXEC fail on lazy expired WATCHed key in tests/unit/multi.tcl
Expected '1' to be equal to '' (context: type eval line 13 cmd {assert_equal [r exec] {}} proc ::test)

When WATCH is called on a key that's already logically expired, avoid discarding the transaction when the keys is actually deleted.

    test {EXEC fail on lazy expired WATCHed key} {
        r flushall
        r debug set-active-expire 0

        r del key
        r set key 1 px 2
        r watch key  ------> key already logically expired

        after 100

        r multi
        r incr key
        assert_equal [r exec] {}
        r debug set-active-expire 1
    } {OK} {needs:debug}

new edit:
ok, i have an alternative method (#10332)