access to old value during notification of expiry

[daniel-house]

Currently, when a key expires during the active-expiry-cycle, notifications are sent at https://github.com/redis/redis/blob/5705cec68e2b93ace879cc1c8d05aca19bcfd188/src/db.c#L1455

I am working on a module that needs access to the value of the key immediately before the key is expired. It would be helpful if the notifications were either delivered first or delivered with the old value. Delivery before the actual deletion seems superior because it costs nothing when the subscriber does not need the old value.

[oranagra]

On the other hand, I imagine there may be cases when someone depends on getting a notification on an event after it happened, so the effect on the action can already be seen. I.E. This is correct for all notifications, the receiver may not specifically handle each of them.

@MeirShpilraien @guybe7 WDYT?

[MeirShpilraien]

Maybe we can add another notification, expiring ? that is given just before the key is actually deleted?

[oranagra]

we could add an additional set of notifications (just for modules) on keyspace changes before they happen. specifically for ones that delete the keys (expiry, eviction, etc), for other notifications it nay not be that simple..

[guybe7]

LGTM (adding a pre-deletion notification for modules)

on the other hand, I don't think we currently have any notifications that are "just for modules" so there's no precedence not saying this is the way to go but we could use module events (example: we have pre-load and post-load events for persistence - we could have pre-delete and post-delete for keys)

[MeirShpilraien]

@guybe7 we do have a notification which is just for modules, the loaded notification.

[daniel-house]

I like pre and post notifications. I have seen them work well together in other situations.

[oranagra]

ok, @daniel-house do you wanna make a PR?

[daniel-house]

I apologize for not following this more closely.

I don't think an unlink-notification is what I need. Perhaps what I need can be built on top of this, but I do not yet see how to do so. An unlink may occur synchronously or asynchronously - when it occurs asynchronously the key has become unavailable for an unspecified length of time. I need to get the event just before the delete is scheduled, not when it actually occurs. I can see how my phrase "Delivery before the actual deletion ..." was not sufficiently precise. Also, I need to know that the reason for the delete is due to expiry. I am aware that there may still be a significant amount of time between the TTL and the actual expiry, but see no reason to add the time between scheduling the unlink and the time when the unlink actually happens.

Currently the code looks like this:

/* Delete the specified expired key and propagate expire. */
void deleteExpiredKeyAndPropagate(redisDb *db, robj *keyobj) {
    mstime_t expire_latency;
    latencyStartMonitor(expire_latency);
    if (server.lazyfree_lazy_expire)
        dbAsyncDelete(db,keyobj);
    else
        dbSyncDelete(db,keyobj);
    latencyEndMonitor(expire_latency);
    latencyAddSampleIfNeeded("expire-del",expire_latency);
    notifyKeyspaceEvent(NOTIFY_EXPIRED,"expired",keyobj,db->id);
    signalModifiedKey(NULL, db, keyobj);
    propagateExpire(db,keyobj,server.lazyfree_lazy_expire);
    server.stat_expiredkeys++;
}

Notice that the current EXPIRED event is sent immediately after the delete is scheduled - not at the time the unlink actually occurs.

I'm not looking for any change in the semantics of the EXPIRE event. I.e., today the EXPIRE event is delivered in the master but is NOT delivered in its replicas. Although the function name propagateExpire suggests otherwise, it actually propagates a DEL command to the replicas, which is processed quite differently from the actual expiry in the master. This should not change, and the PRE_EXPIRE event should not be delivered to the module in the replicas.

I need something that would behave almost exactly the same, so it might look like this:

/* Delete the specified expired key and propagate expire. */
void deleteExpiredKeyAndPropagate(redisDb *db, robj *keyobj) {
    mstime_t expire_latency;
    notifyKeyspaceEvent(NOTIFY_PRE_EXPIRED,"pre_expired",keyobj,db->id); // ONE LINE INSERTED, NO OTHER CHANGE!
    latencyStartMonitor(expire_latency);
    if (server.lazyfree_lazy_expire)
        dbAsyncDelete(db,keyobj);
    else
        dbSyncDelete(db,keyobj);
    latencyEndMonitor(expire_latency);
    latencyAddSampleIfNeeded("expire-del",expire_latency);
    notifyKeyspaceEvent(NOTIFY_EXPIRED,"expired",keyobj,db->id);
    signalModifiedKey(NULL, db, keyobj);
    propagateExpire(db,keyobj,server.lazyfree_lazy_expire);
    server.stat_expiredkeys++;
}

Thus I would need a totally new event, PRE_EXPIRE, and the callback would need to be able to see the contents of the key, but, like any callback, Warning: the notification callbacks are performed in a synchronous manner, so notification callbacks must to be fast, or they would slow Redis down. If you need to take long actions, use threads to offload them.

If the value of the key has a reference counter, the callback should be able to increment that counter (e.g., use RedisModule_RetainString) so that the value can be passed to a thread without the cost of actually copying it. It would probably be a good idea to allow the callback to retain the key-string that it receives.

[oranagra]

@daniel-house one thing i don't think i understand from your post above. the new REMOVED notifications in #9406 is always synchronous, even when the deletion will happen in the background, the unlinking of that key from the database happens at the moment we detected that it expired (see the call to dbAsyncDelete your post, that's the one we modified in the PR.

regarding the ability to distinguish between deletion due to expiration, and deletion to to other reasons, i don't know how generic that request is.... it seems logical to let modules probe keys before they're deleted, but i don't want to add something that's very specific to expiration. maybe we can find a way to always send some reason hint to that callback, so you'll be able to ignore the calls with any reasons other than expiration.

[daniel-house]

Thanks for the correction. After reading dbAsyncDelete more carefully I see that it always calls moduleNotifyKeyUnlink after finding the entry and before deciding when the value should be freed.

A "reason hint" would be sufficient. I'd rather not see reasons other than expiration, but seeing them and ignoring them will work.

[oranagra]

@daniel-house how about being able to probe the key's TTL and seeing it's in the past? i.e. instead of a reason hint?

@guybe7 @MeirShpilraien do you think this kind of hint is useful for other modules? we don't have a way to pass hints to the keyspace notification event other than either modify the event string, or the type enum. i.e. instead of adding a single REDISMODULE_NOTIFY_REMOVED, we can add EXPIRED, EVICTED, and DELETED, as 3 separate events. or we can fire just one REDISMODULE_NOTIFY_REMOVED event, but set the event string to "removed-expired", "removed-evicted", and "removed-deleted".

any other ideas?

[guybe7]

even probing the TTL won't suffice because it's possible that a volatile key was deleted by DEL and by the time we probe the TTL it's already expired

I don't like providing a reason for the deletion, it's a bit too specific and where does it stop? someone may ask for deletions that were made in an implicit way (last SPOP for example)

@daniel-house what about using the new "removed" notification, read whatever you need from the key, and add it to a list/hash? then process it when you get an "expire" event of a key that exists in that hash/list? with some occasional house cleaning of this struct, removing "stale" entries (keys that were deleted, but not by expire)

[daniel-house]

The REDISMODULE_NOTIFY_REMOVED event makes sense to me. It covers all types of deletes, and I can see how that would be useful. I'd probably rename it to REDISMODULE_NOTIFY_PRE_REMOVE because for me, REMOVED implies that is has already happened while PRE_REMOVE implies that it is just about to happen.

I emphasized preserving the semantics of the REDISMODULE_NOTIFY_EXPIRED event because that event never occurs in a replica, which is important to the module I am working on. A hack that tests if this is the master or a replica would probably work but it smells wrong to me.

Delivering every REMOVED event and then ignoring the ones that are not due to expiry seems wasteful. It would work for me so long as there was an easy and reliable way to determine which ones were due to expiry.

When the idea of pre-delete and post-delete events was suggested my first thought was "oh, yeah, a pre-event flag", because I could see many existing events that could be delivered before or after the action occurs, or, if the user wanted, both before and after. That would be more generic.

Building a complicated system that retains key-value information from just before it is removed until it is either deleted or expired (or evicted?) smells expensive and fragile.

Adding one new event and inserting one line in the deleteExpiredKeyAndPropagate just seems so easy.

Instead, adding a pre-api such as RedisModule_SubscribeToPreKeyspaceEvents and a pre flag to the RedisModuleKeyspaceSubscriber structure also seems easy but would change more code. There wouldn't be any new events but each event for which the pre flag makes sense would need an appropriate code change and the pre-api should probably prevent subscribing to pre-events that are not yet supported. This might not be the best design for a pre-api but I hope it suggests some new ideas.

Another thought: if we ever add interceptors, it would be good to be able to intercept events in much the same way as we intercept commands. A uniform approach to both would be very powerful.

[oranagra]

even probing the TTL won't suffice because it's possible that a volatile key was deleted by DEL and by the time we probe the TTL it's already expired

that's not really a problem, the expiration will happen before the deletion, and it'll actually be considered an expiration.

@daniel-house it's a little hard to follow your thoughts, please state what do you suggest. i think that a generic "pre" event for all keyspace notifications looks too complicated at this point.

personally, i think adding a reason flag for the new PRE_REMOVED event is the right way. that can either be done by splitting this event into several events (different "enum" values), or by appending the accompanied string. i think we can have 4 values for that hint: EVICTED, EXPIRED, DELETED, and IMPLICIT (last one used in SPOP and alike)