Merge pull request #10355 from oranagra/release-7.0-rc27.0-rc2

Release 7.0 RC2

1 files changed, 133 insertions, 41 deletions

diff --git a/src/db.c b/src/db.c
index 218eaf567..d28349664 100644
--- a/src/db.c
+++ b/src/db.c
@@ -83,16 +83,16 @@ robj *lookupKey(redisDb *db, robj *key, int flags) {
     robj *val = NULL;
     if (de) {
         val = dictGetVal(de);
-        int force_delete_expired = flags & LOOKUP_WRITE;
-        if (force_delete_expired) {
-            /* Forcing deletion of expired keys on a replica makes the replica
-             * inconsistent with the master. The reason it's allowed for write
-             * commands is to make writable replicas behave consistently. It
-             * shall not be used in readonly commands. Modules are accepted so
-             * that we don't break old modules. */
-            client *c = server.in_script ? scriptGetClient() : server.current_client;
-            serverAssert(!c || !c->cmd || (c->cmd->flags & (CMD_WRITE|CMD_MODULE)));
-        }
+        /* Forcing deletion of expired keys on a replica makes the replica
+         * inconsistent with the master. We forbid it on readonly replicas, but
+         * we have to allow it on writable replicas to make write commands
+         * behave consistently.
+         *
+         * It's possible that the WRITE flag is set even during a readonly
+         * command, since the command may trigger events that cause modules to
+         * perform additional writes. */
+        int is_ro_replica = server.masterhost && server.repl_slave_ro;
+        int force_delete_expired = flags & LOOKUP_WRITE && !is_ro_replica;
         if (expireIfNeeded(db, key, force_delete_expired)) {
             /* The key is no longer valid. */
             val = NULL;
@@ -1340,6 +1340,11 @@ int dbSwapDatabases(int id1, int id2) {
     redisDb aux = server.db[id1];
     redisDb *db1 = &server.db[id1], *db2 = &server.db[id2];
 
+    /* Swapdb should make transaction fail if there is any
+     * client watching keys */
+    touchAllWatchedKeysInDb(db1, db2);
+    touchAllWatchedKeysInDb(db2, db1);
+
     /* Swap hash tables. Note that we don't swap blocking_keys,
      * ready_keys and watched_keys, since we want clients to
      * remain in the same DB they were. */
@@ -1361,14 +1366,9 @@ int dbSwapDatabases(int id1, int id2) {
      * However normally we only do this check for efficiency reasons
      * in dbAdd() when a list is created. So here we need to rescan
      * the list of clients blocked on lists and signal lists as ready
-     * if needed.
-     *
-     * Also the swapdb should make transaction fail if there is any
-     * client watching keys */
+     * if needed. */
     scanDatabaseForReadyLists(db1);
-    touchAllWatchedKeysInDb(db1, db2);
     scanDatabaseForReadyLists(db2);
-    touchAllWatchedKeysInDb(db2, db1);
     return C_OK;
 }
 
@@ -1387,6 +1387,10 @@ void swapMainDbWithTempDb(redisDb *tempDb) {
         redisDb aux = server.db[i];
         redisDb *activedb = &server.db[i], *newdb = &tempDb[i];
 
+        /* Swapping databases should make transaction fail if there is any
+         * client watching keys. */
+        touchAllWatchedKeysInDb(activedb, newdb);
+
         /* Swap hash tables. Note that we don't swap blocking_keys,
          * ready_keys and watched_keys, since clients 
          * remain in the same DB they were. */
@@ -1408,12 +1412,8 @@ void swapMainDbWithTempDb(redisDb *tempDb) {
          * However normally we only do this check for efficiency reasons
          * in dbAdd() when a list is created. So here we need to rescan
          * the list of clients blocked on lists and signal lists as ready
-         * if needed.
-         *
-         * Also the swapdb should make transaction fail if there is any
-         * client watching keys. */
+         * if needed. */
         scanDatabaseForReadyLists(activedb);
-        touchAllWatchedKeysInDb(activedb, newdb);
     }
 
     trackingInvalidateKeysOnFlush(1);
@@ -1692,7 +1692,7 @@ int64_t getAllKeySpecsFlags(struct redisCommand *cmd, int inv) {
 /* Fetch the keys based of the provided key specs. Returns the number of keys found, or -1 on error.
  * There are several flags that can be used to modify how this function finds keys in a command.
  * 
- * GET_KEYSPEC_INCLUDE_CHANNELS: Return channels as if they were keys.
+ * GET_KEYSPEC_INCLUDE_NOT_KEYS: Return 'fake' keys as if they were keys.
  * GET_KEYSPEC_RETURN_PARTIAL:   Skips invalid and incomplete keyspecs but returns the keys
  *                               found in other valid keyspecs. 
  */
@@ -1703,8 +1703,8 @@ int getKeysUsingKeySpecs(struct redisCommand *cmd, robj **argv, int argc, int se
     for (j = 0; j < cmd->key_specs_num; j++) {
         keySpec *spec = cmd->key_specs + j;
         serverAssert(spec->begin_search_type != KSPEC_BS_INVALID);
-        /* Skip specs that represent channels instead of keys */
-        if ((spec->flags & CMD_KEY_CHANNEL) && !(search_flags & GET_KEYSPEC_INCLUDE_CHANNELS)) {
+        /* Skip specs that represent 'fake' keys */
+        if ((spec->flags & CMD_KEY_NOT_KEY) && !(search_flags & GET_KEYSPEC_INCLUDE_NOT_KEYS)) {
             continue;
         }
 
@@ -1821,31 +1821,123 @@ invalid_spec:
  * associated with how Redis will access the key.
  *
  * 'cmd' must be point to the corresponding entry into the redisCommand
- * table, according to the command name in argv[0].
- *
- * This function uses the command's key specs, which contain the key-spec flags,
- * (e.g. RO / RW) and only resorts to the command-specific helper function if
- * any of the keys-specs are marked as INCOMPLETE. */
+ * table, according to the command name in argv[0]. */
 int getKeysFromCommandWithSpecs(struct redisCommand *cmd, robj **argv, int argc, int search_flags, getKeysResult *result) {
-    if (cmd->flags & CMD_MODULE_GETKEYS) {
+    /* The command has at least one key-spec not marked as NOT_KEY */
+    int has_keyspec = (getAllKeySpecsFlags(cmd, 1) & CMD_KEY_NOT_KEY);
+    /* The command has at least one key-spec marked as VARIABLE_FLAGS */
+    int has_varflags = (getAllKeySpecsFlags(cmd, 0) & CMD_KEY_VARIABLE_FLAGS);
+
+    /* Flags indicating that we have a getkeys callback */
+    int has_module_getkeys = cmd->flags & CMD_MODULE_GETKEYS;
+    int has_native_getkeys = !(cmd->flags & CMD_MODULE) && cmd->getkeys_proc;
+
+    /* The key-spec that's auto generated by RM_CreateCommand sets VARIABLE_FLAGS since no flags are given.
+     * If the module provides getkeys callback, we'll prefer it, but if it didn't, we'll use key-spec anyway. */
+    if ((cmd->flags & CMD_MODULE) && has_varflags && !has_module_getkeys)
+        has_varflags = 0;
+
+    /* We prefer key-specs if there are any, and their flags are reliable. */
+    if (has_keyspec && !has_varflags) {
+        int ret = getKeysUsingKeySpecs(cmd,argv,argc,search_flags,result);
+        if (ret >= 0)
+            return ret;
+        /* If the specs returned with an error (probably an INVALID or INCOMPLETE spec),
+         * fallback to the callback method. */
+    }
+
+    /* Resort to getkeys callback methods. */
+    if (has_module_getkeys)
         return moduleGetCommandKeysViaAPI(cmd,argv,argc,result);
-    } else {
-        if (!(getAllKeySpecsFlags(cmd, 0) & CMD_KEY_VARIABLE_FLAGS)) {
-            int ret = getKeysUsingKeySpecs(cmd,argv,argc,search_flags,result);
-            if (ret >= 0)
-                return ret;
-        }
-        if (!(cmd->flags & CMD_MODULE) && cmd->getkeys_proc)
-            return cmd->getkeys_proc(cmd,argv,argc,result);
-        return 0;
-    }
+
+    /* We use native getkeys as a last resort, since not all these native getkeys provide
+     * flags properly (only the ones that correspond to INVALID, INCOMPLETE or VARIABLE_FLAGS do.*/
+    if (has_native_getkeys)
+        return cmd->getkeys_proc(cmd,argv,argc,result);
+    return 0;
 }
 
 /* This function returns a sanity check if the command may have keys. */
 int doesCommandHaveKeys(struct redisCommand *cmd) {
     return (!(cmd->flags & CMD_MODULE) && cmd->getkeys_proc) || /* has getkeys_proc (non modules) */
         (cmd->flags & CMD_MODULE_GETKEYS) ||                    /* module with GETKEYS */
-        (getAllKeySpecsFlags(cmd, 1) & CMD_KEY_CHANNEL);       /* has at least one key-spec not marked as CHANNEL */
+        (getAllKeySpecsFlags(cmd, 1) & CMD_KEY_NOT_KEY);        /* has at least one key-spec not marked as NOT_KEY */
+}
+
+/* A simplified channel spec table that contains all of the redis commands
+ * and which channels they have and how they are accessed. */
+typedef struct ChannelSpecs {
+    redisCommandProc *proc; /* Command procedure to match against */
+    uint64_t flags;         /* CMD_CHANNEL_* flags for this command */
+    int start;              /* The initial position of the first channel */
+    int count;              /* The number of channels, or -1 if all remaining
+                             * arguments are channels. */
+} ChannelSpecs;
+
+ChannelSpecs commands_with_channels[] = {
+    {subscribeCommand, CMD_CHANNEL_SUBSCRIBE, 1, -1},
+    {ssubscribeCommand, CMD_CHANNEL_SUBSCRIBE, 1, -1},
+    {unsubscribeCommand, CMD_CHANNEL_UNSUBSCRIBE, 1, -1},
+    {sunsubscribeCommand, CMD_CHANNEL_UNSUBSCRIBE, 1, -1},
+    {psubscribeCommand, CMD_CHANNEL_PATTERN | CMD_CHANNEL_SUBSCRIBE, 1, -1},
+    {punsubscribeCommand, CMD_CHANNEL_PATTERN | CMD_CHANNEL_UNSUBSCRIBE, 1, -1},
+    {publishCommand, CMD_CHANNEL_PUBLISH, 1, 1},
+    {spublishCommand, CMD_CHANNEL_PUBLISH, 1, 1},
+    {NULL,0} /* Terminator. */
+};
+
+/* Returns 1 if the command may access any channels matched by the flags
+ * argument. */
+int doesCommandHaveChannelsWithFlags(struct redisCommand *cmd, int flags) {
+    /* If a module declares get channels, we are just going to assume
+     * has channels. This API is allowed to return false positives. */
+    if (cmd->flags & CMD_MODULE_GETCHANNELS) {
+        return 1;
+    }
+    for (ChannelSpecs *spec = commands_with_channels; spec->proc != NULL; spec += 1) {
+        if (cmd->proc == spec->proc) {
+            return !!(spec->flags & flags);
+        }
+    }
+    return 0;
+}
+
+/* Return all the arguments that are channels in the command passed via argc / argv. 
+ * This function behaves similar to getKeysFromCommandWithSpecs, but with channels 
+ * instead of keys.
+ * 
+ * The command returns the positions of all the channel arguments inside the array,
+ * so the actual return value is a heap allocated array of integers. The
+ * length of the array is returned by reference into *numkeys.
+ * 
+ * Along with the position, this command also returns the flags that are
+ * associated with how Redis will access the channel.
+ *
+ * 'cmd' must be point to the corresponding entry into the redisCommand
+ * table, according to the command name in argv[0]. */
+int getChannelsFromCommand(struct redisCommand *cmd, robj **argv, int argc, getKeysResult *result) {
+    keyReference *keys;
+    /* If a module declares get channels, use that. */
+    if (cmd->flags & CMD_MODULE_GETCHANNELS) {
+        return moduleGetCommandChannelsViaAPI(cmd, argv, argc, result);
+    }
+    /* Otherwise check the channel spec table */
+    for (ChannelSpecs *spec = commands_with_channels; spec != NULL; spec += 1) {
+        if (cmd->proc == spec->proc) {
+            int start = spec->start;
+            int stop = (spec->count == -1) ? argc : start + spec->count;
+            if (stop > argc) stop = argc;
+            int count = 0;
+            keys = getKeysPrepareResult(result, stop - start);
+            for (int i = start; i < stop; i++ ) {
+                keys[count].pos = i;
+                keys[count++].flags = spec->flags;
+            }
+            result->numkeys = count;
+            return count;
+        }
+    }
+    return 0;
 }
 
 /* The base case is to use the keys position as given in the command table