1. 21 Aug, 2022 1 commit
  2. 18 Aug, 2022 4 commits
    • Binbin's avatar
      Fix CLUSTERDOWN issue in cluster reshard unblock test (#11139) · 3a16ad30
      Binbin authored
      change the cluster-node-timeout from 1 to 1000
      3a16ad30
    • guybe7's avatar
      Repurpose redisCommandArg's name as the unique ID (#11051) · 223046ec
      guybe7 authored
      This PR makes sure that "name" is unique for all arguments in the same
      level (i.e. all args of a command and all args within a block/oneof).
      This means several argument with identical meaning can be referred to together,
      but also if someone needs to refer to a specific one, they can use its full path.
      
      In addition, the "display_text" field has been added, to be used by redis.io
      in order to render the syntax of the command (for the vast majority it is
      identical to "name" but sometimes we want to use a different string
      that is not "name")
      The "display" field is exposed via COMMAND DOCS and will be present
      for every argument, except "oneof" and "block" (which are container
      arguments)
      
      Other changes:
      1. Make sure we do not have any container arguments ("oneof" or "block")
         that contain less than two sub-args (otherwise it doesn't make sense)
      2. migrate.json: both AUTH and AUTH2 should not be "optional"
      3. arg names cannot contain un...
      223046ec
    • Binbin's avatar
      Fix memory leak in moduleFreeCommand (#11147) · fc3956e8
      Binbin authored
      Currently, we call zfree(cmd->args), but the argument array
      needs to be freed recursively (there might be sub-args).
      Also fixed memory leaks on cmd->tips and cmd->history.
      
      Fixes #11145
      fc3956e8
    • Meir Shpilraien (Spielrein)'s avatar
      Fix replication inconsistency on modules that uses key space notifications (#10969) · 508a1388
      Meir Shpilraien (Spielrein) authored
      Fix replication inconsistency on modules that uses key space notifications.
      
      ### The Problem
      
      In general, key space notifications are invoked after the command logic was
      executed (this is not always the case, we will discuss later about specific
      command that do not follow this rules). For example, the `set x 1` will trigger
      a `set` notification that will be invoked after the `set` logic was performed, so
      if the notification logic will try to fetch `x`, it will see the new data that was written.
      Consider the scenario on which the notification logic performs some write
      commands. for example, the notification logic increase some counter,
      `incr x{counter}`, indicating how many times `x` was changed.
      The logical order by which the logic was executed is has follow:
      
      ```
      set x 1
      incr x{counter}
      ```
      
      The issue is that the `set x 1` command is added to the replication buffer
      at the end of the command invocation (specifically after the key space
      notification logic was invoked and performed the `incr` command).
      The replication/aof sees the commands in the wrong order:
      
      ```
      incr x{counter}
      set x 1
      ```
      
      In this specific example the order is less important.
      But if, for example, the notification would have deleted `x` then we would
      end up with primary-replica inconsistency.
      
      ### The Solution
      
      Put the command that cause the notification in its rightful place. In the
      above example, the `set x 1` command logic was executed before the
      notification logic, so it should be added to the replication buffer before
      the commands that is invoked by the notification logic. To achieve this,
      without a major code refactoring, we save a placeholder in the replication
      buffer, when finishing invoking the command logic we check if the command
      need to be replicated, and if it does, we use the placeholder to add it to the
      replication buffer instead of appending it to the end.
      
      To be efficient and not allocating memory on each command to save the
      placeholder, the replication buffer array was modified to reuse memory
      (instead of allocating it each time we want to replicate commands).
      Also, to avoid saving a placeholder when not needed, we do it only for
      WRITE or MAY_REPLICATE commands.
      
      #### Additional Fixes
      
      * Expire and Eviction notifications:
        * Expire/Eviction logical order was to first perform the Expire/Eviction
          and then the notification logic. The replication buffer got this in the
          other way around (first notification effect and then the `del` command).
          The PR fixes this issue.
        * The notification effect and the `del` command was not wrap with
          `multi-exec` (if needed). The PR also fix this issue.
      * SPOP command:
        * On spop, the `spop` notification was fired before the command logic
          was executed. The change in this PR would have cause the replication
          order to be change (first `spop` command and then notification `logic`)
          although the logical order is first the notification logic and then the
          `spop` logic. The right fix would have been to move the notification to
          be fired after the command was executed (like all the other commands),
          but this can be considered a breaking change. To overcome this, the PR
          keeps the current behavior and changes the `spop` code to keep the right
          logical order when pushing commands to the replication buffer. Another PR
          will follow to fix the SPOP properly and match it to the other command (we
          split it to 2 separate PR's so it will be easy to cherry-pick this PR to 7.0 if
          we chose to).
      
      #### Unhanded Known Limitations
      
      * key miss event:
        * On key miss event, if a module performed some write command on the
          event (using `RM_Call`), the `dirty` counter would increase and the read
          command that cause the key miss event would be replicated to the replication
          and aof. This problem can also happened on a write command that open
          some keys but eventually decides not to perform any action. We decided
          not to handle this problem on this PR because the solution is complex
          and will cause additional risks in case we will want to cherry-pick this PR.
          We should decide if we want to handle it in future PR's. For now, modules
          writers is advice not to perform any write commands on key miss event.
      
      #### Testing
      
      * We already have tests to cover cases where a notification is invoking write
        commands that are also added to the replication buffer, the tests was modified
        to verify that the replica gets the command in the correct logical order.
      * Test was added to verify that `spop` behavior was kept unchanged.
      * Test was added to verify key miss event behave as expected.
      * Test was added to verify the changes do not break lazy expiration.
      
      #### Additional Changes
      
      * `propagateNow` function can accept a special dbid, -1, indicating not
        to replicate `select`. We use this to replicate `multi/exec` on `propagatePendingCommands`
        function. The side effect of this change is that now the `select` command
        will appear inside the `multi/exec` block on the replication stream (instead of
        outside of the `multi/exec` block). Tests was modified to match this new behavior.
      508a1388
  3. 16 Aug, 2022 1 commit
  4. 15 Aug, 2022 1 commit
  5. 14 Aug, 2022 5 commits
    • guybe7's avatar
      Rename offset and xsetid tags (#11103) · 1189680e
      guybe7 authored
      There's really no point in having dedicated flags to test these features
      (why shouldn't all commands/features get their own tag?)
      1189680e
    • kmy2001's avatar
      Optimization in t_hash.c: Avoid looking for a same field twice by using... · eef2d830
      kmy2001 authored
      Optimization in t_hash.c: Avoid looking for a same field twice by using dictAddRaw() instead of dictFind() and dictAdd() (#11110)
      
      Before this change in hashTypeSet() function, we first use dictFind()
      to look for the field and if it does not exist, we use dictAdd() to add it.
      In dictAdd() function the dictionary will look for the field again and I
      think this is meaningless as we already know that the field does not exist.
      
      An optimization is to use dictAddRaw() instead of dictFind() and dictAdd().
      If we use dictAddRaw(), a new entry will be added when the field does not
      exist, and what we should do then is just set the value of that entry, and set
      its key to 'sdsdup(field)' in the case that 'HASH_SET_TAKE_FIELD' flag wasn't set.
      eef2d830
    • Ozan Tezcan's avatar
      Fix Lua compile warning on GCC 12.1 (#11115) · c5ff163d
      Ozan Tezcan authored
      Fix Lua compile warning on GCC 12.1
      
      GCC 12.1 prints a warning on compile: 
      ```
      ldump.c: In function ‘DumpString’:
      ldump.c:63:26: warning: the comparison will always evaluate as ‘false’ for the pointer operand in ‘s + 24’ must not be NULL [-Waddress]
         63 |  if (s==NULL || getstr(s)==NULL)
      
      ```
      
      It seems correct, `getstr(s)` can't be `NULL`.  
      Also, I see Lua v5.2 does not have that check: https://github.com/lua/lua/blob/v5-2/ldump.c#L63
      c5ff163d
    • sundb's avatar
      Add missing lua_pop in luaGetFromRegistry (#11097) · 8aad2ac3
      sundb authored
      This pr mainly has the following four changes:
      
      1. Add missing lua_pop in `luaGetFromRegistry`.
          This bug affects `redis.register_function`, where `luaGetFromRegistry` in
          `luaRegisterFunction` will return null when we call `redis.register_function` nested.
          .e.g
          ```
          FUNCTION LOAD "#!lua name=mylib \n local lib=redis \n lib.register_function('f2', function(keys, args) lib.register_function('f1', function () end) end)"
          fcall f2 0
          ````
          But since we exit when luaGetFromRegistry returns null, it does not cause the stack to grow indefinitely.
      
      3. When getting `REGISTRY_RUN_CTX_NAME` from the registry, use `serverAssert`
          instead of error return. Since none of these lua functions are registered at the time
          of function load, scriptRunCtx will never be NULL.
      4. Add `serverAssert` for `luaLdbLineHook`, `luaEngineLoadHook`.
      5. Remove `luaGetFromRegistry` from `redis_math_random` and
          `redis_math_randomseed`, it looks like they are redundant.
      8aad2ac3
    • Binbin's avatar
      Fix outdated lfu-decay-time doc in redis.conf (#11108) · 1f600efd
      Binbin authored
      The divided by two and less <= 10 logics were changed in 06ca9d68.
      Now we just decrement the counter by num_periods.
      
      The lfu-decay-time special value of 0 's meaning was actually changed in 06ca9d68.
      Now we won't do anything on counter if lfu-decay-time is 0.
      1f600efd
  6. 11 Aug, 2022 1 commit
    • Ozan Tezcan's avatar
      Fix overflow in redis-benchmark (#11102) · 99ebbee2
      Ozan Tezcan authored
      Fix overflow in redis-benchmark affecting latency measurements on 32bit builds.
      
      If `long` is 4 bytes (typical on 32 bit systems), multiplication overflows.
      Using `long long` will fix the issue as it is guaranteed to be at least 8 bytes. 
      
      Also, I've added a change to reuse `ustime()` for `mstime()`. 
      99ebbee2
  7. 10 Aug, 2022 1 commit
  8. 09 Aug, 2022 1 commit
  9. 07 Aug, 2022 2 commits
  10. 05 Aug, 2022 1 commit
  11. 04 Aug, 2022 4 commits
    • Binbin's avatar
      Re-enable aof-race integration tests (#10972) · 6a7dd00c
      Binbin authored
      This is the history of aof-race related changes:
      1. added in 3aa4b009
      2. disabled in dcdfd005
      3. enabled in 5c639226
      4. disabled in 53a2af39
      
      This PR refreshes the aof-race test, re-enable it.
      Closes #10971
      6a7dd00c
    • Binbin's avatar
      errno cleanup around rdbLoad (#11042) · 4505eb18
      Binbin authored
      This is an addition to #11039, which cleans up rdbLoad* related errno. Remove the
      errno print from the outer message (may be invalid since errno may have been overwritten).
      
      Our aim should be the code that detects the error and knows which system call
      triggered it, is the one to print errno, and not the code way up above (in some cases
      a result of a logical error and not a system one).
      
      Remove the code to update errno in rdbLoadRioWithLoadingCtx, signature check
      and the rdb version check, in these cases, we do print the error message.
      The caller dose not have the specific logic for handling EINVAL.
      
      Small fix around rdb-preamble AOF: A truncated RDB is considered a failure,
      not handled the same as a truncated AOF file.
      4505eb18
    • filipe oliveira's avatar
      Avoid the sdslen() on shared.crlf given we know its size beforehand. Improve... · 6686c6d7
      filipe oliveira authored
      Avoid the sdslen() on shared.crlf given we know its size beforehand. Improve ~3-4% of cpu cycles to lrange logic (#10987)
      
      * Avoid the sdslen() on shared.crlf given we know its size beforehand
      * Removed shared.crlf from sharedObjects
      6686c6d7
    • Jie Liang Ang's avatar
      Reuse checkGoodReplicasStatus in script.c (#11078) · f3588fbc
      Jie Liang Ang authored
      Small refactoring done to reuse `checkGoodReplicasStatus` in `script.c` when checking for status of good replicas.
      f3588fbc
  12. 03 Aug, 2022 4 commits
  13. 02 Aug, 2022 2 commits
  14. 01 Aug, 2022 2 commits
  15. 31 Jul, 2022 2 commits
  16. 28 Jul, 2022 1 commit
    • Binbin's avatar
      Avoid false positive out-of-bounds in writeForgottenNodePingExt (#11053) · 90f35cea
      Binbin authored
      In clusterMsgPingExtForgottenNode, sizeof(name) is CLUSTER_NAMELEN,
      and sizeof(clusterMsgPingExtForgottenNode) is > CLUSTER_NAMELEN.
      Doing a (name + sizeof(clusterMsgPingExtForgottenNode)) sanitizer
      generates an out-of-bounds error which is a false positive in here
      90f35cea
  17. 27 Jul, 2022 2 commits
    • Binbin's avatar
      Fix bgsaveerr issue in psync wrong offset test (#11043) · e7144693
      Binbin authored
      The kill above is sometimes successful and sometimes already too late.
      The PING in pysnc wrong offset test got rejected by bgsaveerr because
      lastbgsave_status is C_ERR.
      
      In theory, using diskless can avoid PING being affected, because when
      the replica is dropped, we will kill the child with SIGUSR1, and this
      will not affect lastbgsave_status.
      
      Anyway, this kill is not particularly needed here, dropping the kill
      is the best one, since we do have the waitForBgsave, so just let it
      take care of the bgsave. No need for fast termination.
      e7144693
    • guybe7's avatar
      Adds RM_Microseconds and RM_CachedMicroseconds (#11016) · 45c99d70
      guybe7 authored
      RM_Microseconds
      Return the wall-clock Unix time, in microseconds
      
      RM_CachedMicroseconds
      Returns a cached copy of the Unix time, in microseconds.
      It is updated in the server cron job and before executing a command.
      It is useful for complex call stacks, such as a command causing a
      key space notification, causing a module to execute a RedisModule_Call,
      causing another notification, etc.
      It makes sense that all these callbacks would use the same clock.
      45c99d70
  18. 26 Jul, 2022 4 commits
    • Binbin's avatar
      Change the return value of rdbLoad function to enums (#11039) · 00097bf4
      Binbin authored
      The reason we do this is because in #11036, we added error
      log message when failing to open RDB file for reading.
      In loadDdataFromDisk we call rdbLoad and also check errno,
      now the logging corrupts errno (reported in alpine daily).
      
      It is not safe to rely on errno as we do today, so we change
      the return value of rdbLoad function to enums, like we have
      when loading an AOF.
      00097bf4
    • Huang Zhw's avatar
      When client tracking is on, invalidation message of flushdb in a (#11038) · 6f0a27e3
      Huang Zhw authored
      When FLUSHDB / FLUSHALL / SWAPDB is inside MULTI / EXEC, the
      client side tracking invalidation message was interleaved with transaction response.
      6f0a27e3
    • Meir Shpilraien (Spielrein)'s avatar
      Fix #11030, use lua_rawget to avoid triggering metatables and crash. (#11032) · 020e046b
      Meir Shpilraien (Spielrein) authored
      Fix #11030, use lua_rawget to avoid triggering metatables.
      
      #11030 shows how return `_G` from the Lua script (either function or eval), cause the
      Lua interpreter to Panic and the Redis processes to exit with error code 1.
      Though return `_G` only panic on Redis 7 and 6.2.7, the underline issue exists on older
      versions as well (6.0 and 6.2). The underline issue is returning a table with a metatable
      such that the metatable raises an error.
      
      The following example demonstrate the issue:
      ```
      127.0.0.1:6379> eval "local a = {}; setmetatable(a,{__index=function() foo() end}) return a" 0
      Error: Server closed the connection
      ```
      ```
      PANIC: unprotected error in call to Lua API (user_script:1: Script attempted to access nonexistent global variable 'foo')
      ```
      
      The Lua panic happened because when returning the result to the client, Redis needs to
      introspect the returning table and transform the table into a resp. In order to scan the table,
      Redis uses `lua_gettable` api which might trigger the metatable (if exists) and might raise an error.
      This code is not running inside `pcall` (Lua protected call), so raising an error causes the
      Lua to panic and exit. Notice that this is not a crash, its a Lua panic that exit with error code 1.
      
      Returning `_G` panics on Redis 7 and 6.2.7 because on those versions `_G` has a metatable
      that raises error when trying to fetch a none existing key.
      
      ### Solution
      
      Instead of using `lua_gettable` that might raise error and cause the issue, use `lua_rawget`
      that simply return the value from the table without triggering any metatable logic.
      This is promised not to raise and error.
      
      The downside of this solution is that it might be considered as breaking change, if someone
      rely on metatable in the returned value. An alternative solution is to wrap this entire logic
      with `pcall` (Lua protected call), this alternative require a much bigger refactoring.
      
      ### Back Porting
      
      The same fix will work on older versions as well (6.2, 6.0). Notice that on those version,
      the issue can cause Redis to crash if inside the metatable logic there is an attempt to accesses
      Redis (`redis.call`). On 7.0, there is not crash and the `redis.call` is executed as if it was done
      from inside the script itself.
      
      ### Tests
      
      Tests was added the verify the fix
      020e046b
    • Viktor Söderqvist's avatar
      Gossip forgotten nodes on `CLUSTER FORGET` (#10869) · 5032de50
      Viktor Söderqvist authored
      Gossip the cluster node blacklist in ping and pong messages.
      This means that CLUSTER FORGET doesn't need to be sent to all nodes in a cluster.
      It can be sent to one or more nodes and then be propagated to the rest of them.
      
      For each blacklisted node, its node id and its remaining blacklist TTL is gossiped in a
      cluster bus ping extension (introduced in #9530).
      5032de50
  19. 25 Jul, 2022 1 commit