Changes and fixes for the new DELIFEQ command (#2120)

DELIFEQ was added in #1975 but had some issues.

Do some refactoring to reduce the number of lines of code,
write commands need to increment dirty and we can propagate
it as DEL command.

Changes:
1. Replicate as DEL is important for compatibility with replicas
   running an older Valkey version, also it can save the compare
   logic, like other commands like GETDEL.
2. Signal modified key is for WATCH and do invalidate client-side
   caching (client tracking)
3. Keyspace notifications.
4. Dirty++ indicates there are unsaved changes for RDB.
5. Using shared strings for the RESP zero and one replies for the
   code cleanup.
6. Add FAST command flag and remove the ACCESS key specs flag.

Signed-off-by: Binbin <[email protected]>

Author: enjoy-binbin

src/commands.def

@@ -10474,7 +10474,7 @@ struct COMMAND_ARG DECRBY_Args[] = {
 #ifndef SKIP_CMD_KEY_SPECS_TABLE
 /* DELIFEQ key specs */
 keySpec DELIFEQ_Keyspecs[1] = {
-{NULL,CMD_KEY_RM|CMD_KEY_ACCESS|CMD_KEY_DELETE,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}}
+{NULL,CMD_KEY_RM|CMD_KEY_DELETE,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}}
 };
 #endif
 
@@ -11341,7 +11341,7 @@ struct COMMAND_STRUCT serverCommandTable[] = {
 {MAKE_CMD("append","Appends a string to the value of a key. Creates the key if it doesn't exist.","O(1). The amortized time complexity is O(1) assuming the appended value is small and the already present value is of any size, since the dynamic string library used by the server will double the free space available on every reallocation.","2.0.0",CMD_DOC_NONE,NULL,NULL,"string",COMMAND_GROUP_STRING,APPEND_History,0,APPEND_Tips,0,appendCommand,3,CMD_WRITE|CMD_DENYOOM|CMD_FAST,ACL_CATEGORY_STRING,APPEND_Keyspecs,1,NULL,2),.args=APPEND_Args},
 {MAKE_CMD("decr","Decrements the integer value of a key by one. Uses 0 as initial value if the key doesn't exist.","O(1)","1.0.0",CMD_DOC_NONE,NULL,NULL,"string",COMMAND_GROUP_STRING,DECR_History,0,DECR_Tips,0,decrCommand,2,CMD_WRITE|CMD_DENYOOM|CMD_FAST,ACL_CATEGORY_STRING,DECR_Keyspecs,1,NULL,1),.args=DECR_Args},
 {MAKE_CMD("decrby","Decrements a number from the integer value of a key. Uses 0 as initial value if the key doesn't exist.","O(1)","1.0.0",CMD_DOC_NONE,NULL,NULL,"string",COMMAND_GROUP_STRING,DECRBY_History,0,DECRBY_Tips,0,decrbyCommand,3,CMD_WRITE|CMD_DENYOOM|CMD_FAST,ACL_CATEGORY_STRING,DECRBY_Keyspecs,1,NULL,2),.args=DECRBY_Args},
-{MAKE_CMD("delifeq","Delete key if value matches string.","O(1)","9.0.0",CMD_DOC_NONE,NULL,NULL,"string",COMMAND_GROUP_STRING,DELIFEQ_History,0,DELIFEQ_Tips,0,delifeqCommand,3,CMD_WRITE,ACL_CATEGORY_STRING,DELIFEQ_Keyspecs,1,NULL,2),.args=DELIFEQ_Args},
+{MAKE_CMD("delifeq","Delete key if value matches string.","O(1)","9.0.0",CMD_DOC_NONE,NULL,NULL,"string",COMMAND_GROUP_STRING,DELIFEQ_History,0,DELIFEQ_Tips,0,delifeqCommand,3,CMD_FAST|CMD_WRITE,ACL_CATEGORY_STRING,DELIFEQ_Keyspecs,1,NULL,2),.args=DELIFEQ_Args},
 {MAKE_CMD("get","Returns the string value of a key.","O(1)","1.0.0",CMD_DOC_NONE,NULL,NULL,"string",COMMAND_GROUP_STRING,GET_History,0,GET_Tips,0,getCommand,2,CMD_READONLY|CMD_FAST,ACL_CATEGORY_STRING,GET_Keyspecs,1,NULL,1),.args=GET_Args},
 {MAKE_CMD("getdel","Returns the string value of a key after deleting the key.","O(1)","6.2.0",CMD_DOC_NONE,NULL,NULL,"string",COMMAND_GROUP_STRING,GETDEL_History,0,GETDEL_Tips,0,getdelCommand,2,CMD_WRITE|CMD_FAST,ACL_CATEGORY_STRING,GETDEL_Keyspecs,1,NULL,1),.args=GETDEL_Args},
 {MAKE_CMD("getex","Returns the string value of a key after setting its expiration time.","O(1)","6.2.0",CMD_DOC_NONE,NULL,NULL,"string",COMMAND_GROUP_STRING,GETEX_History,0,GETEX_Tips,0,getexCommand,-2,CMD_WRITE|CMD_FAST,ACL_CATEGORY_STRING,GETEX_Keyspecs,1,NULL,2),.args=GETEX_Args},

src/commands/delifeq.json

@@ -7,6 +7,7 @@
         "arity": 3,
         "function": "delifeqCommand",
         "command_flags": [
+            "FAST",
             "WRITE"
         ],
         "acl_categories": [
@@ -16,7 +17,6 @@
             {
                 "flags": [
                     "RM",
-                    "ACCESS",
                     "DELETE"
                 ],
                 "begin_search": {

src/t_string.c

@@ -384,30 +384,24 @@ void psetexCommand(client *c) {
     setGenericCommand(c, OBJ_PX | OBJ_ARGV3, c->argv[1], c->argv[3], c->argv[2], UNIT_MILLISECONDS, NULL, NULL, NULL);
 }
 
+/* DELIFEQ key value */
 void delifeqCommand(client *c) {
-    robj *existing_value = lookupKeyWrite(c->db, c->argv[1]);
-
-    if (existing_value == NULL) {
-        addReplyLongLong(c, 0);
-        return;
-    }
-
-    if (checkType(c, existing_value, OBJ_STRING)) {
-        /* Error reply already sent */
-        return;
-    }
+    robj *o;
+    if ((o = lookupKeyWriteOrReply(c, c->argv[1], shared.czero)) == NULL || checkType(c, o, OBJ_STRING)) return;
 
-    if (compareStringObjects(existing_value, c->argv[2]) != 0) {
-        addReplyLongLong(c, 0);
+    if (compareStringObjects(o, c->argv[2]) != 0) {
+        addReply(c, shared.czero);
         return;
     }
 
-    if (!dbSyncDelete(c->db, c->argv[1])) {
-        addReplyLongLong(c, 0);
-        return;
-    }
+    serverAssert(dbSyncDelete(c->db, c->argv[1]));
 
-    addReplyLongLong(c, 1);
+    /* Propagate as DEL command */
+    rewriteClientCommandVector(c, 2, shared.del, c->argv[1]);
+    signalModifiedKey(c, c->db, c->argv[1]);
+    notifyKeyspaceEvent(NOTIFY_GENERIC, "del", c->argv[1], c->db->id);
+    server.dirty++;
+    addReply(c, shared.cone);
 }
 
 int getGenericCommand(client *c) {

tests/unit/type/string.tcl

@@ -777,6 +777,18 @@ if {[string match {*jemalloc*} [s mem_allocator]]} {
         assert_error "WRONGTYPE*" {r delifeq foo "test"}
     }
 
+    test {DELIFEQ propagate as DEL command to replica} {
+        set repl [attach_to_replication_stream]
+        r set foo bar
+        r delifeq foo bar
+        assert_replication_stream $repl {
+            {select *}
+            {set foo bar}
+            {del foo}
+        }
+        close_replication_stream $repl
+    } {} {needs:repl}
+
 if {[string match {*jemalloc*} [s mem_allocator]]} {
     test {Memory usage of embedded string value} {
         # Check that we can fit 9 bytes of key + value into a 32 byte