EXISTS should not alter LRU, OBJECT should not reveal expired keys on replica (#8016)

guybe7 · web-flow · commit f8ae991717f1 · 2020-11-18T11:16:21.000+02:00
The bug was introduced by #5021 which only attempted avoid EXIST on an already expired key from returning 1 on a replica. Before that commit, dbExists was used instead of lookupKeyRead (which had an undesired effect to "touch" the LRU/LFU) Other than that, this commit fixes OBJECT to also come empty handed on expired keys in replica. And DEBUG DIGEST-VALUE to behave like DEBUG OBJECT (get the data from the key regardless of it's expired state)
diff --git a/src/db.c b/src/db.c
@@ -102,11 +102,8 @@ robj *lookupKeyReadWithFlags(redisDb *db, robj *key, int flags) {
         /* Key expired. If we are in the context of a master, expireIfNeeded()
          * returns 0 only when the key does not exist at all, so it's safe
          * to return NULL ASAP. */
-        if (server.masterhost == NULL) {
-            server.stat_keyspace_misses++;
-            notifyKeyspaceEvent(NOTIFY_KEY_MISS, "keymiss", key, db->id);
-            return NULL;
-        }
+        if (server.masterhost == NULL)
+            goto keymiss;
 
         /* However if we are in the context of a slave, expireIfNeeded() will
          * not really try to expire the key, it only returns information
@@ -125,19 +122,21 @@ robj *lookupKeyReadWithFlags(redisDb *db, robj *key, int flags) {
             server.current_client->cmd &&
             server.current_client->cmd->flags & CMD_READONLY)
         {
-            server.stat_keyspace_misses++;
-            notifyKeyspaceEvent(NOTIFY_KEY_MISS, "keymiss", key, db->id);
-            return NULL;
+            goto keymiss;
         }
     }
     val = lookupKey(db,key,flags);
-    if (val == NULL) {
+    if (val == NULL)
+        goto keymiss;
+    server.stat_keyspace_hits++;
+    return val;
+
+keymiss:
+    if (!(flags & LOOKUP_NONOTIFY)) {
         server.stat_keyspace_misses++;
         notifyKeyspaceEvent(NOTIFY_KEY_MISS, "keymiss", key, db->id);
     }
-    else
-        server.stat_keyspace_hits++;
-    return val;
+    return NULL;
 }
 
 /* Like lookupKeyReadWithFlags(), but does not use any flag, which is the
@@ -592,7 +591,7 @@ void existsCommand(client *c) {
     int j;
 
     for (j = 1; j < c->argc; j++) {
-        if (lookupKeyRead(c->db,c->argv[j])) count++;
+        if (lookupKeyReadWithFlags(c->db,c->argv[j],LOOKUP_NOTOUCH)) count++;
     }
     addReplyLongLong(c,count);
 }
diff --git a/src/debug.c b/src/debug.c
@@ -644,7 +644,11 @@ NULL
         for (int j = 2; j < c->argc; j++) {
             unsigned char digest[20];
             memset(digest,0,20); /* Start with a clean result */
-            robj *o = lookupKeyReadWithFlags(c->db,c->argv[j],LOOKUP_NOTOUCH);
+
+            /* We don't use lookupKey because a debug command should
+             * work on logically expired keys */
+            dictEntry *de;
+            robj *o = ((de = dictFind(c->db->dict,c->argv[j]->ptr)) == NULL) ? NULL : dictGetVal(de);
             if (o) xorObjectDigest(c->db,c->argv[j],digest,o);
 
             sds d = sdsempty();
diff --git a/src/object.c b/src/object.c
@@ -1229,10 +1229,7 @@ int objectSetLRUOrLFU(robj *val, long long lfu_freq, long long lru_idle,
 /* This is a helper function for the OBJECT command. We need to lookup keys
  * without any modification of LRU or other parameters. */
 robj *objectCommandLookup(client *c, robj *key) {
-    dictEntry *de;
-
-    if ((de = dictFind(c->db->dict,key->ptr)) == NULL) return NULL;
-    return (robj*) dictGetVal(de);
+    return lookupKeyReadWithFlags(c->db,key,LOOKUP_NOTOUCH|LOOKUP_NONOTIFY);
 }
 
 robj *objectCommandLookupOrReply(client *c, robj *key, robj *reply) {
diff --git a/src/server.h b/src/server.h
@@ -2179,6 +2179,7 @@ int objectSetLRUOrLFU(robj *val, long long lfu_freq, long long lru_idle,
                        long long lru_clock, int lru_multiplier);
 #define LOOKUP_NONE 0
 #define LOOKUP_NOTOUCH (1<<0)
+#define LOOKUP_NONOTIFY (1<<1)
 void dbAdd(redisDb *db, robj *key, robj *val);
 int dbAddRDBLoad(redisDb *db, sds key, robj *val);
 void dbOverwrite(redisDb *db, robj *key, robj *val);
diff --git a/tests/unit/introspection-2.tcl b/tests/unit/introspection-2.tcl
@@ -3,11 +3,12 @@ proc cmdstat {cmd} {
 }
 
 start_server {tags {"introspection"}} {
-    test {TTL and TYPYE do not alter the last access time of a key} {
+    test {TTL, TYPE and EXISTS do not alter the last access time of a key} {
         r set foo bar
         after 3000
         r ttl foo
         r type foo
+        r exists foo
         assert {[r object idletime foo] >= 2}
     }
 

Original file line number	Diff line number	Diff line change
`@@ -3,11 +3,12 @@ proc cmdstat {cmd} {`
`3`	`3`	`}`
`4`	`4`
`5`	`5`	`start_server {tags {"introspection"}} {`
`6`		`- test {TTL and TYPYE do not alter the last access time of a key} {`
	`6`	`+ test {TTL, TYPE and EXISTS do not alter the last access time of a key} {`
`7`	`7`	`r set foo bar`
`8`	`8`	`after 3000`
`9`	`9`	`r ttl foo`
`10`	`10`	`r type foo`
	`11`	`+ r exists foo`
`11`	`12`	`assert {[r object idletime foo] >= 2}`
`12`	`13`	`}`
`13`	`14`