Incr expired_keys if the expiration time is already expired #1517

RayaCoo · 2025-01-07T02:43:36Z

This PR modifies two parts of the deleteExpiredKeyFromOverwriteAndPropagate function, which is called when the expiration time has already expired.

change Keyspace Event notify from GENERIC-del to EXPIRED-expired.
Since this delete operation is triggered by EXPIRE, and the flag pass to dbGenericDeleteWithDictIndex is DB_FLAG_KEY_EXPIRED, which is used to notify module, maybe we should align the behavior and send Keyspace Event notify like deleteExpiredKeyAndPropagateWithDictIndex did.
As i said befoce, this del operation is triggered by EXPIRE, maybe increasing server.stat_expiredkeys in this case could more accurately reflect the number of keys deleted due to expiration.

codecov · 2025-01-07T02:57:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.85%. Comparing base (c0014ef) to head (11348fe).
Report is 32 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1517      +/-   ##
============================================
+ Coverage     70.82%   70.85%   +0.03%     
============================================
  Files           120      120              
  Lines         64911    64915       +4     
============================================
+ Hits          45972    45995      +23     
+ Misses        18939    18920      -19

Files with missing lines	Coverage Δ
src/db.c	`89.55% <100.00%> (+<0.01%)`	⬆️

... and 17 files with indirect coverage changes

soloestoy · 2025-01-07T06:03:28Z

The core of this issue is whether we should consider setting an expired time for a key as a delete operation or an expiration.

I believe it should be considered as expiration because the user's intended action is to set an expire time for the key. However, due to some reasons (such as operational delays), the time has already passed. Nonetheless, the key is still deleted due to expiration. Therefore, in terms of statistics and notifications, it should be counted as expiration-based deletion.

Additionally, we have already sent notifications regarding the expiration for the module:

void deleteExpiredKeyFromOverwriteAndPropagate(client *c, robj *keyobj) {
    int deleted = dbGenericDelete(c->db, keyobj, server.lazyfree_lazy_expire, DB_FLAG_KEY_EXPIRED);

int dbGenericDelete(serverDb *db, robj *key, int async, int flags) {
    int dict_index = getKVStoreIndexForKey(key->ptr);
    return dbGenericDeleteWithDictIndex(db, key, async, flags, dict_index);
}

int dbGenericDeleteWithDictIndex(serverDb *db, robj *key, int async, int flags, int dict_index) {
    hashtablePosition pos;
    void **ref = kvstoreHashtableTwoPhasePopFindRef(db->keys, dict_index, key->ptr, &pos);
    if (ref != NULL) {
        robj *val = *ref;
        /* VM_StringDMA may call dbUnshareStringValue which may free val, so we
         * need to incr to retain val */
        incrRefCount(val);
        /* Tells the module that the key has been unlinked from the database. */
        moduleNotifyKeyUnlink(key, val, db->id, flags);

However, there is a change here: the notification has been changed from GENERIC-del to EXPIRED-expired. This might be a potential breaking change, but theoretically, the actual impact should be minimal. Typically, if there's a concern about a key being deleted, the generic, expired, and evicted events would all be subscribed to. @valkey-io/core-team PTAL

Signed-off-by: Ray Cao <[email protected]>

enjoy-binbin

overall make sense to me.

tests/unit/expire.tcl

Signed-off-by: Ray Cao <[email protected]>

madolson · 2025-01-07T22:25:06Z

This might be a potential breaking change, but theoretically, the actual impact should be minimal.

Yeah, I think at the very least this needs to be marked as a breaking change, since we are changing the event type. Even if the new behavior is more correct, it's such a long standing behavior someone might have coded around it.

soloestoy · 2025-01-13T07:01:44Z

since it's a breaking change (very small), @valkey-io/core-team please vote

zuiderkwast · 2025-01-13T09:42:39Z

src/db.c

@@ -1860,7 +1860,8 @@ void deleteExpiredKeyFromOverwriteAndPropagate(client *c, robj *keyobj) {
    robj *aux = server.lazyfree_lazy_expire ? shared.unlink : shared.del;
    rewriteClientCommandVector(c, 2, aux, keyobj);
    signalModifiedKey(c, c->db, keyobj);
-    notifyKeyspaceEvent(NOTIFY_GENERIC, "del", keyobj, c->db->id);
+    notifyKeyspaceEvent(NOTIFY_EXPIRED, "expired", keyobj, c->db->id);


~~What's the difference between expire and expired event?~~ I think the difference between "expire" and "expired" is clear in the docs.

In the docs I can find for "expired":

Every time a key with a time to live associated is removed from the data set because it expired, an expired event is generated.

and for "expire" event:

EXPIRE and all its variants (PEXPIRE, EXPIREAT, PEXPIREAT) generate an expire event when called with a positive timeout (or a future timestamp). Note that when these com‐
mands are called with a negative timeout value or timestamp in the past, the key is deleted and only a del event is generated instead.

This seems to me like a bug fix. A key expiration is supposed to trigger an expired event. A del event seems like a bug. Or am I missing anything?

you can find "expired" in active expire path: activeExpireCycleTryExpire -> deleteExpiredKeyAndPropagate -> deleteExpiredKeyAndPropagateWithDictIndex

Thanks. Yes I found it.

Shall we postpone the breaking change to 9.0?

i think 8.1 is ok, it's a very very small change maybe even not breaking.

If we do it in 8.1, we can describe it as a bug fix. It looks better than a breaking change in a minor version.

But let's be sure we know what we are doing. We have done some mistakes before such as #1537 and #1228. We often think that small things like that will not be a problem.

@madolson said

Even if the new behavior is more correct, it's such a long standing behavior someone might have coded around it.

I'm very sure what it is : )

I'm very sure what it is : )

What is it exactly?

If it's a breaking change, we shouldn't release it in a minor version.

Breaking change → release in 9.0.

Not breaking change → release in 8.1 and remove the breaking-change label.

This is a bugfix, but it might also introduce a breaking change. These two are not mutually exclusive, and I don't understand why it has to be an either-or situation.

Because we use semantic versioning. It's the only reason.

Major version must be incremented if we introduce backward incompatible API changes.

If the public documented API is not changed, then we are just fixing incorrect behaviour. Then a patch release is enough.

https://semver.org

OK, this PR doesn't introduce API changes, I removed the breaking-change flag, and 8.1 is the right way

ranshid · 2025-01-14T05:05:04Z

Added the client-changes-needed since it MIGHT impact some client code managing client side caching

hwware · 2025-01-15T16:03:43Z

The change totally makes sense to me. Approved

…d other commands(valkey-io#1517) Some commands that use unix-time, such as `EXPIREAT` and `SET EXAT`, should include the deleted keys in the `expired_keys` statistics if the specified time has already expired, and notifications should be sent in the manner of expired. --------- Signed-off-by: Ray Cao <[email protected]>

add expired_keys statistic if the expiration time is already expired

aafe746

Signed-off-by: Ray Cao <[email protected]>

RayaCoo force-pushed the expire-del-statistic branch from 18efebb to aafe746 Compare January 7, 2025 07:18

enjoy-binbin approved these changes Jan 7, 2025

View reviewed changes

tests/unit/expire.tcl Outdated Show resolved Hide resolved

tests/unit/expire.tcl Outdated Show resolved Hide resolved

make test more stable

11348fe

Signed-off-by: Ray Cao <[email protected]>

enjoy-binbin added breaking-change Indicates a possible backwards incompatible change release-notes This issue should get a line item in the release notes major-decision-pending Major decision pending by TSC team labels Jan 8, 2025

soloestoy approved these changes Jan 13, 2025

View reviewed changes

zuiderkwast reviewed Jan 13, 2025

View reviewed changes

ranshid added the client-changes-needed Client changes are required for this feature label Jan 14, 2025

soloestoy removed major-decision-pending Major decision pending by TSC team breaking-change Indicates a possible backwards incompatible change labels Jan 16, 2025

soloestoy merged commit 921ba19 into valkey-io:unstable Jan 16, 2025
50 checks passed

RayaCoo deleted the expire-del-statistic branch January 17, 2025 03:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incr expired_keys if the expiration time is already expired #1517

Incr expired_keys if the expiration time is already expired #1517

RayaCoo commented Jan 7, 2025

codecov bot commented Jan 7, 2025 •

edited

Loading

soloestoy commented Jan 7, 2025

enjoy-binbin left a comment

madolson commented Jan 7, 2025

soloestoy commented Jan 13, 2025

zuiderkwast Jan 13, 2025 •

edited

Loading

soloestoy Jan 13, 2025

zuiderkwast Jan 13, 2025

soloestoy Jan 13, 2025

zuiderkwast Jan 13, 2025

soloestoy Jan 13, 2025

zuiderkwast Jan 15, 2025

soloestoy Jan 16, 2025

zuiderkwast Jan 16, 2025

soloestoy Jan 16, 2025

ranshid commented Jan 14, 2025

hwware commented Jan 15, 2025

Incr expired_keys if the expiration time is already expired #1517

Incr expired_keys if the expiration time is already expired #1517

Conversation

RayaCoo commented Jan 7, 2025

codecov bot commented Jan 7, 2025 • edited Loading

Codecov Report

soloestoy commented Jan 7, 2025

enjoy-binbin left a comment

Choose a reason for hiding this comment

madolson commented Jan 7, 2025

soloestoy commented Jan 13, 2025

zuiderkwast Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranshid commented Jan 14, 2025

hwware commented Jan 15, 2025

codecov bot commented Jan 7, 2025 •

edited

Loading

zuiderkwast Jan 13, 2025 •

edited

Loading