1. 30 Jan, 2024 5 commits
    • Chen Tianjie's avatar
      Add novalues option to command HSCAN. (#12765) · f469dd8c
      Chen Tianjie authored
      
      
      Add a way to HSCAN a hash key, and get only the filed names.
      Command syntax is now:
      ```
      HSCAN key cursor [MATCH pattern] [COUNT count] [NOVALUES]
      ```
      when `NOVALUES` is on, the command will only return keys in the hash.
      
      ---------
      Co-authored-by: default avatarViktor Söderqvist <viktor.soderqvist@est.tech>
      f469dd8c
    • Slava Koyfman's avatar
      Implement `CLIENT KILL MAXAGE <maxage>` (#12299) · 24f6d08b
      Slava Koyfman authored
      
      
      Adds an ability to kill clients older than a specified age.
      
      Also, fixed the age calculation in `catClientInfoString` to use
      `commandTimeSnapshot`
      instead of the old `server.unixtime`, and added missing documentation
      for
      `CLIENT KILL ID` to output of `CLIENT help`.
      
      ---------
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      24f6d08b
    • Binbin's avatar
      Fix timeout not being set in module blockClient case (#13011) · 45a35a79
      Binbin authored
      This was introduced in #13004, missing this assignment.
      It causes timeout to be a random value (may be less than now),
      and then in `Unblock by timer` test, the client is unblocked
      and then it call timeout_callback, since the callback is NULL,
      the server will crash.
      
      The crash stack is:
      ```
      beforesleep
      handleBlockedClientsTimeout
      checkBlockedClientTimeout
      unblockClientOnTimeout
      replyToBlockedClientTimedOut
      moduleBlockedClientTimedOut
      -- the timeout_callback is NULL, invalidFunctionWasCalled
      bc->timeout_callback(&ctx,(void**)c->argv,c->argc);
      ```
      45a35a79
    • Binbin's avatar
      Adds connection timeout option to redis-cli (#10609) · 76adbf6f
      Binbin authored
      
      
      This allows specifying the timeout value for opening the TCP
      connection to a server. The timeout, default 0 means no limit,
      depending on the OS. It can be specified using the new `-t` switch.
      
      revive #3764, fixes #3763
      
      ---------
      Co-authored-by: default avatarItamar Haber <itamar@redislabs.com>
      Co-authored-by: default avataryoav-steinberg <yoav@redislabs.com>
      76adbf6f
    • Binbin's avatar
      Fix blocking commands timeout is reset due to re-processing command (#13004) · 492021db
      Binbin authored
      In #11012, we will reprocess command when client is unblocked on keys,
      in some blocking commands, for example, in the XREADGROUP BLOCK
      scenario,
      because of the re-processing command, we will recalculate the block
      timeout,
      causing the blocking time to be reset.
      
      This commit add a new CLIENT_REPROCESSING_COMMAND clent flag, explicitly
      let the command know that it is being re-processed, later in
      blockForKeys
      we will not reset the timeout.
      
      Affected BLOCK cases: 
      - list / zset / stream, added test cases for each.
      
      Unaffected cases:
      - module (never re-process the commands).
      - WAIT / WAITAOF (never re-process the commands).
      
      Fixes #12998.
      492021db
  2. 29 Jan, 2024 3 commits
    • Chen Tianjie's avatar
      Optimize resizing hash table to resize not only non-empty dicts. (#12819) · af7ceeb7
      Chen Tianjie authored
      The function `tryResizeHashTables` only attempts to shrink the dicts
      that has keys (change from #11695), this was a serious problem until the
      change in #12850 since it meant if all keys are deleted, we won't shrink
      the dick.
      But still, both dictShrink and dictExpand may be blocked by a fork child
      process, therefore, the cron job needs to perform both dictShrink and
      dictExpand, for not just non-empty dicts, but all dicts in DBs.
      
      What this PR does:
      
      1. Try to resize all dicts in DBs (not just non-empty ones, as it was
      since #12850)
      2. handle both shrink and expand (not just shrink, as it was since
      forever)
      3. Refactor some APIs about dict resizing (get rid of `htNeedsShrink`
      `htNeedsShrink` `dictShrinkToFit`, and expose `dictShrinkIfNeeded`
      `dictExpandIfNeeded` which already contains all the code of those
      functions we get rid of, to make APIs more neat)
      4. In the `Don't rehash if redis has child process` test, now that cron
      would do resizing, we no longer need to write to DB after the child
      process got killed, and can wait for the cron to expand the hash table.
      af7ceeb7
    • Ozan Tezcan's avatar
      Add RM_TryCalloc() and RM_TryRealloc() (#12985) · c5273cae
      Ozan Tezcan authored
      Modules may want to handle allocation failures gracefully. Adding
      RM_TryCalloc() and RM_TryRealloc() for it.
      RM_TryAlloc() was added before:
      https://github.com/redis/redis/pull/10541
      c5273cae
    • Binbin's avatar
      Fix maxmemory-samples stack overflow crash in evictionPoolPopulate, limit its... · acd96052
      Binbin authored
      Fix maxmemory-samples stack overflow crash in evictionPoolPopulate, limit its value to [1,64] (#13000)
      
      We have not limited the value of maxmemory-samples in the past, it can
      be set very large. If it is set very large, we will have stack overflow
      in evictionPoolPopulate when we trigger the key eviction.
      
      There is no reason for this config to be set too high, so just limit its
      range to [1,64].
      acd96052
  3. 27 Jan, 2024 1 commit
    • Roshan Khatri's avatar
      Reduce performance impact of dict rehashing and make it shorter. (#12899) · 5358bd7c
      Roshan Khatri authored
      
      
      #### Problem Statement:
      For any read/update operation during rehashing, we're doing ~10+ random
      DRAM lookups to do the rehashing, as we are using the `rehashidx` to
      rehash 10 buckets, whose dict entries most likely aren't cached in the
      CPU or near the bucket we are operating on. If these random bucket are
      empty, the rehashing process during that command execution is skipped.
      
      #### Implementation:
      For reducing the performance recession while dict is rehashing, we
      determine the index at which the key would be stored in the 0th HT, we
      check if that index has already been rehashed, if not we will rehash the
      bucket containing the key and the bucket will be moved from 0th HT to
      the 1st HT.
      
      If the key has already been rehashed, we perform the random access
      bucket rehash (using `rehashidx`) and we again verify if rehashing is
      still ongoing and look up the key in the respective HT.
      
      This ensures rehashing is not skipped in any command call and that we
      rehash a particular bucket or random bucket in each call.
      
      #### Changes in this PR:
      - Added a new method `dictBucketRehash` to perform rehash on a single
      bucket.
      - Helper function `moveKeysInBucketOldtoNew` for `dictRehash` and
      `dictBucketRehash` to move all the keys in a bucket from the old to the
      new hash HT.
      - Helper function `verifyMoreRehashRequired` for `dictRehash` and
      `dictBucketRehash` to check if we have already rehashed the whole table
      and if more rehashing is required.
      
      ### Benchmark:
      - This PR still shows **~13%** improvement in the latency during
      rehashing.
      
      - Rehashing is now **~2%** faster for this PR when compared to unstable.
      
      ---------
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      Co-authored-by: default avatarMadelyn Olson <34459052+madolson@users.noreply.github.com>
      5358bd7c
  4. 26 Jan, 2024 1 commit
  5. 25 Jan, 2024 2 commits
    • Binbin's avatar
      Fix unauthenticated client query buffer 1MB limit (#12989) · 4cb5ad85
      Binbin authored
      Code incorrectly set the limit value to 1024MB.
      Introduced in #12961.
      4cb5ad85
    • zhaozhao.zz's avatar
      Revert multi OOM limit and add multi buffer limit (#12961) · 85a834bf
      zhaozhao.zz authored
      Fix #9926 , and introduce an alternative method to prevent abuse of
      transactions:
      
      1. revert #5454 (which was blocking read-only transactions in OOM
      state), and break the tie of MULTI state memory usage and the server OOM
      state. Meaning that we'll limit the total memory a single client can
      queue, and do that unconditionally regardless of the server being OOM or
      not.
      2. to prevent abuse of transactions, we use the
      `client-query-buffer-limit` to restrict the size of the transaction.
      Because the commands cached in the MULTI/EXEC queue have not been
      executed yet, so they are also considered a part of the "query buffer"
      in a broader sense. In other words, the commands in the MULTI queue and
      the `querybuf` of the client together constitute the "query buffer".
      When they exceed the limit, the connection will be disconnected.
      
      The reasoning is that it's sensible to sends a single command with a
      huge (1GB) argument, and it's sensible to sends a transaction with many
      small commands, but it's probably not common to sends a long transaction
      with many huge arguments (will consume a lot of memory before even being
      executed).
      
      If anyone runs into that, they can simply increase the
      `client-query-buffer-limit` config.
      
      P.S. To prevent DDoS attacks, unauthenticated clients have a separate
      hard limit. Their query buffer should not exceed a maximum of 1MB. In
      other words, if the query buffer of an unauthenticated client exceeds
      1MB or the `client-query-buffer-limit` (if it is set to a value smaller
      than 1MB,), the connection will be disconnected.
      85a834bf
  6. 23 Jan, 2024 5 commits
    • Binbin's avatar
      Add sender NULL check in clusterProcessGossipSection invalid_ids case (#12980) · 07b292af
      Binbin authored
      In the following case sender may be unknown, so we need to set up a
      NULL check for sender:
      ```
      /* If this is a MEET packet from an unknown node, we still process
       * the gossip section here since we have to trust the sender because
       * of the message type. */
      if (!sender && type == CLUSTERMSG_TYPE_MEET)
          clusterProcessGossipSection(hdr,link);
      ```
      07b292af
    • Binbin's avatar
      Allow running WAITAOF in scripts, remove NOSCRIPT flag (#12977) · 85c31e0c
      Binbin authored
      In #11568 we removed the NOSCRIPT flag from commands, e.g. removing
      NOSCRIPT flag from WAIT. Aiming to allow them in scripts and let them
      implicitly behave in the non-blocking way.
      
      This PR remove NOSCRIPT flag from WAITAOF just like WAIT (to be
      symmetrical)).
      And this PR also add BLOCKING flag for WAIT and WAITAOF.
      85c31e0c
    • Binbin's avatar
      Some cleanups around function (#12940) · 628c0dea
      Binbin authored
      This PR did some cleanups around function:
      - drop the comment about Libraries Ctx, since we do have comment
        in functionsLibCtx, no need to maintain multiple copies.
      - remove outdated comment about the dropped Library description.
      - remove unused desc and code vars in functionExtractLibMetaData.
      - fix engines_nemory typo, changed it to engines_memory.
      - remove outdated comment about FUNCTION CREATE and FUNCTION INFO,
        FUNCTION CREATE was renamed to FUNCTION LOAD.
      - Check in initServer whether the return of functionsInit is OK.
      628c0dea
    • Oran Agra's avatar
      update redis-check-rdb types (#12969) · f9a0eb60
      Oran Agra authored
      seems that we forgot to update the array in redis-check rdb.
      f9a0eb60
    • Harkrishn Patro's avatar
      Exit early if slowlog/acllog max len set to zero (#12965) · 2bce71b5
      Harkrishn Patro authored
      Currently slowlog gets disabled if slowlog-log-slower-than is set to less than zero. I think we should also disable it if slowlog-max-len is set to zero. We apply the same logic to acllog-max-len.
      2bce71b5
  7. 22 Jan, 2024 2 commits
    • Brennan's avatar
      Prevent nodes with invalid IDs from being propagated through gossip (#12921) · e12f2dec
      Brennan authored
      
      
      There have been occasional instances of memory corruption (though code bugs or bit flips) leading to invalid node information being gossiped around. To prevent this invalid information spreading, we verify the node IDs in received gossip are in an acceptable format, and disregard any gossiped nodes with invalid IDs. This PR uses the existing verifyClusterNodeId function to check the validity of the gossiped node IDs and if an invalid one is encountered, logs raw byte information to help debug the corruption.
      
      ---------
      Co-authored-by: default avatarMadelyn Olson <madelyneolson@gmail.com>
      e12f2dec
    • zhaozhao.zz's avatar
      Set the correct id for tempDb (#12947) · 8d0156eb
      zhaozhao.zz authored
      background: some modules need to know the `dbid` information, such as
      the function used during RDB loading:
      
      ```
      robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) {
      ....
              moduleInitIOContext(io,mt,rdb,&keyobj,dbid);
      ```
      
      However, during replication, the "tempDb" created for diskless RDB
      loading is not correctly set with the dbid. This leads to passing the
      wrong dbid to the `rdbLoadObject` function (as tempDb uses zcalloc, all
      ids are 0).
      
      ```
      disklessLoadInitTempDb()->rdbLoadRioWithLoadingCtx()->
              /* Read value */
              val = rdbLoadObject(type,rdb,key,db->id,&error);
      ```
      
      To fix it, set the correct ID (relative index) for the tempdb.
      8d0156eb
  8. 19 Jan, 2024 3 commits
    • Yanqi Lv's avatar
      Change dictGetSafeIterator to dictGetIterator in pubsub (#12931) · 85a239b3
      Yanqi Lv authored
      In #12838, we misuse the safe iterator of the client dict, so we can't
      catch the synchronous release of the client if there is a bug.
      
      Since we realize that clients (even subscribers) are released with async
      free, we change the safe iterators of the client dict into unsafe
      iterators in `pubsub.c`. And I also remove redundant code.
      85a239b3
    • Yanqi Lv's avatar
      Change the threshold of dict expand, shrink and rehash (#12948) · b07174af
      Yanqi Lv authored
      Before this change (most recently modified in
      https://github.com/redis/redis/pull/12850#discussion_r1421406393), The
      trigger for normal expand threshold was 100% utilization and the trigger
      for normal shrink threshold was 10% (HASHTABLE_MIN_FILL).
      While during fork (DICT_RESIZE_AVOID), when we want to avoid rehash, the
      trigger thresholds were multiplied by 5 (`dict_force_resize_ratio`),
      meaning 500% for expand and 2% (100/10/5) for shrink.
      
      However, in `dictRehash` (the incremental rehashing), the rehashing
      threshold for shrinking during fork (DICT_RESIZE_AVOID) was 20% by
      mistake.
      This meant that if a shrinking is triggered when `dict_can_resize` is
      `DICT_RESIZE_ENABLE` which the threshold is 10%, the rehashing can
      continue when `dict_can_resize` is `DICT_RESIZE_AVOID`.
      This would cause unwanted CopyOnWrite damage.
      
      It'll make sense to change the thresholds of the rehash trigger and the
      thresholds of the incremental rehashing the same, however, in one we
      compare the size of the hash table to the number of records, and in the
      other we compare the size of ht[0] to the size of ht[1], so the formula
      is not exactly the same.
      
      to make things easier we change all the thresholds to powers of 2, so
      the normal shrinking threshold is changed from 100/10 (i.e. 10%) to
      100/8 (i.e. 12.5%), and we change the threshold during forks from 5 to
      4, i.e. from 500% to 400% for expand, and from 2% (100/10/5) to 3.125%
      (100/8/4)
      b07174af
    • debing.sun's avatar
      Fix race condition issues between the main thread and module threads (#12817) · d0640029
      debing.sun authored
      Fix #12785 and other race condition issues.
      See the following isolated comments.
      
      The following report was obtained using SANITIZER thread.
      ```sh
      make SANITIZER=thread
      ./runtest-moduleapi --config io-threads 4 --config io-threads-do-reads yes --accurate
      ```
      
      1. Fixed thread-safe issue in RM_UnblockClient()
      Related discussion:
      https://github.com/redis/redis/pull/12817#issuecomment-1831181220
      * When blocking a client in a module using `RM_BlockClientOnKeys()` or
      `RM_BlockClientOnKeysWithFlags()`
      with a timeout_callback, calling RM_UnblockClient() in module threads
      can lead to race conditions
           in `updateStatsOnUnblock()`.
      
           - Introduced: 
              Version: 6.2
              PR: #7491
      
           - Touch:
      `server.stat_numcommands`, `cmd->latency_histogram`, `server.slowlog`,
      and `server.latency_events`
           
           - Harm Level: High
      Potentially corrupts the memory data of `cmd->latency_histogram`,
      `server.slowlog`, and `server.latency_events`
      
           - Solution:
      Differentiate whether the call to moduleBlockedClientTimedOut() comes
      from the module or the main thread.
      Since we can't know if RM_UnblockClient() comes from module threads, we
      always assume it does and
      let `updateStatsOnUnblock()` asynchronously update the unblock status.
           
      * When error reply is called in timeout_callback(), ctx is not
      thread-safe, eventually lead to race conditions in `afterErrorReply`.
      
           - Introduced: 
              Version: 6.2
              PR: #8217
      
           - Touch
             `server.stat_total_error_replies`, `server.errors`, 
      
           - Harm Level: High
             Potentially corrupts the memory data of `server.errors`
         
            - Solution: 
      Make the ctx in `timeout_callback()` with `REDISMODULE_CTX_THREAD_SAFE`,
      and asynchronously reply errors to the client.
      
      2. Made RM_Reply*() family API thread-safe
      Related discussion:
      https://github.com/redis/redis/pull/12817#discussion_r1408707239
      Call chain: `RM_Reply*()` -> `_addReplyToBufferOrList()` -> touch
      server.current_client
      
          - Introduced: 
             Version: 7.2.0
             PR: #12326
      
         - Harm Level: None
      Since the module fake client won't have the `CLIENT_PUSHING` flag, even
      if we touch server.current_client,
           we can still exit after `c->flags & CLIENT_PUSHING`.
      
         - Solution
            Checking `c->flags & CLIENT_PUSHING` earlier.
      
      3. Made freeClient() thread-safe
          Fix #12785
      
          - Introduced: 
             Version: 4.0
      Commit:
      https://github.com/redis/redis/commit/3fcf959e609e850a114d4016843e4c991066ebac
      
          - Harm Level: Moderate
             * Trigger assertion
      It happens when the module thread calls freeClient while the io-thread
      is in progress,
      which just triggers an assertion, and doesn't make any race condiaions.
      
      * Touch `server.current_client`, `server.stat_clients_type_memory`, and
      `clientMemUsageBucket->clients`.
      It happens between the main thread and the module threads, may cause
      data corruption.
      1. Error reset `server.current_client` to NULL, but theoretically this
      won't happen,
      because the module has already reset `server.current_client` to old
      value before entering freeClient.
      2. corrupts `clientMemUsageBucket->clients` in
      updateClientMemUsageAndBucket().
      3. Causes server.stat_clients_type_memory memory statistics to be
      inaccurate.
          
          - Solution:
      * No longer counts memory usage on fake clients, to avoid updating
      `server.stat_clients_type_memory` in freeClient.
      * No longer resetting `server.current_client` in unlinkClient, because
      the fake client won't be evicted or disconnected in the mid of the
      process.
      * Judgment assertion `io_threads_op == IO_THREADS_OP_IDLE` only if c is
      not a fake client.
      
      4. Fixed free client args without GIL
      Related discussion:
      https://github.com/redis/redis/pull/12817#discussion_r1408706695
      When freeing retained strings in the module thread (refcount decr), or
      using them in some way (refcount incr), we should do so while holding
      the GIL,
      otherwise, they might be simultaneously freed while the main thread is
      processing the unblock client state.
      
          - Introduced: 
             Version: 6.2.0
             PR: #8141
      
         - Harm Level: Low
           Trigger assertion or double free or memory leak. 
      
         - Solution:
      Documenting that module API users need to ensure any access to these
      retained strings is done with the GIL locked
      
      5. Fix adding fake client to server.clients_pending_write
          It will incorrectly log the memory usage for the fake client.
      Related discussion:
      https://github.com/redis/redis/pull/12817#issuecomment-1851899163
      
          - Introduced: 
             Version: 4.0
      Commit:
      https://github.com/redis/redis/commit/9b01b64430fbc1487429144d2e4e72a4a7fd9db2
      
      
      
          - Harm Level: None
            Only result in NOP
      
          - Solution:
             * Don't add fake client into server.clients_pending_write
      * Add c->conn assertion for updateClientMemUsageAndBucket() and
      updateClientMemoryUsage() to avoid same
               issue in the future.
      So now it will be the responsibility of the caller of both of them to
      avoid passing in fake client.
      
      6. Fix calling RM_BlockedClientMeasureTimeStart() and
      RM_BlockedClientMeasureTimeEnd() without GIL
          - Introduced: 
             Version: 6.2
             PR: #7491
      
         - Harm Level: Low
      Causes inaccuracies in command latency histogram and slow logs, but does
      not corrupt memory.
      
         - Solution:
      Module API users, if know that non-thread-safe APIs will be used in
      multi-threading, need to take responsibility for protecting them with
      their own locks instead of the GIL, as using the GIL is too expensive.
      
      ### Other issue
      1. RM_Yield is not thread-safe, fixed via #12905.
      
      ### Summarize
      1. Fix thread-safe issues for `RM_UnblockClient()`, `freeClient()` and
      `RM_Yield`, potentially preventing memory corruption, data disorder, or
      assertion.
      2. Updated docs and module test to clarify module API users'
      responsibility for locking non-thread-safe APIs in multi-threading, such
      as RM_BlockedClientMeasureTimeStart/End(), RM_FreeString(),
      RM_RetainString(), and RM_HoldString().
      
      ### About backpot to 7.2
      1. The implement of (1) is not too satisfying, would like to get more
      eyes.
      2. (2), (3) can be safely for backport
      3. (4), (6) just modifying the module tests and updating the
      documentation, no need for a backpot.
      4. (5) is harmless, no need for a backpot.
      
      ---------
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      d0640029
  9. 18 Jan, 2024 4 commits
    • Chen Tianjie's avatar
      Optimize dictTypeResizeAllowed to avoid mistaken OOM judgement. (#12950) · f81c3fd8
      Chen Tianjie authored
      When doing dict resizing, dictTypeResizeAllowed is used to judge whether
      the new allocated memory for rehashing would cause OOM.
      
      However when shrinking, we alloc `_dictNextExp(d->ht_used[0])` bytes of
      memory, while in `dictTypeResizeAllowed` we still use
      `_dictNextExp(d->ht_used[0]+1)` as the new allocated memory size. This
      will overestimate the memory used by shrinking at special conditions,
      causing a false OOM judgement.
      f81c3fd8
    • Binbin's avatar
      Fix minor memory leaks in dictTest (#12962) · 1c7eb0ad
      Binbin authored
      Introduced in #12952, reported by valgrind.
      1c7eb0ad
    • Binbin's avatar
      Call emptyData when disk-based sync rdbLoad fails (#12510) · 0e5a4a27
      Binbin authored
      We doing this in diskless on-empty-db mode, when diskless
      loading fails, we will call emptyData to remove the half-loaded
      data in case we started with an empty replica.
      
      Now when a disk-based sync rdbLoad fails, we will call emptyData
      too in case it loads partially incomplete data.
      
      when the replica attempts another re-sync, it'll empty the dataset
      again anyway, so this affects two things:
      1. memory consumption in the time gap until the next rdb loading begins
      2. if the unsynced replica is for some reason promoted, it would have kept
        the partial dataset instead of being empty.
      0e5a4a27
    • Binbin's avatar
      Fix dict resize ratio checks, avoid precision loss from integer division (#12952) · 14b1edfd
      Binbin authored
      In the past we used integers to compare ratios, let us assume that
      we have the following data in expanding:
      ```
      used / size > 5
      `80 / 16 > 5` is false
      `81 / 16 > 5` is false
      `95 / 16 > 5` is false
      `96 / 16 > 5` is true
      ```
      
      Because the integer result is rounded, our resize breaks the ratio
      constraint, this has existed since the beginning, which resulted in
      us not strictly following the ratio (shrink also has the same issue).
      
      This PR change it to multiplication to avoid floating point
      calculations.
      14b1edfd
  10. 15 Jan, 2024 3 commits
    • Binbin's avatar
      Updated comments on dictResizeEnable for new dict shrink (#12946) · ecc31bc6
      Binbin authored
      The new shrink was added in #12850.
      Also updated outdated comments, see #11692.
      ecc31bc6
    • Yanqi Lv's avatar
      Shrink dict when deleting dictEntry (#12850) · e2b7932b
      Yanqi Lv authored
      When we insert entries into dict, it may autonomously expand if needed.
      However, when we delete entries from dict, it doesn't shrink to the
      proper size. If there are few entries in a very large dict, it may cause
      huge waste of memory and inefficiency when iterating.
      
      The main keyspace dicts (keys and expires), are shrinked by cron
      (`tryResizeHashTables` calls `htNeedsResize` and `dictResize`),
      And some data structures such as zset and hash also do that (call
      `htNeedsResize`) right after a loop of calls to `dictDelete`,
      But many other dicts are completely missing that call (they can only
      expand).
      
      In this PR, we provide the ability to automatically shrink the dict when
      deleting. The conditions triggering the shrinking is the same as
      `htNeedsResize` used to have. i.e. we expand when we're over 100%
      utilization, and shrink when we're below 10% utilization.
      
      Additionally:
      * Add `dictPauseAutoResize` so that flows that do mass deletions, will
      only trigger shrinkage at the end.
      * Rename `dictResize` to `dictShrinkToFit` (same logic as it used to
      have, but better name describing it)
      * Rename `_dictExpand` to `_dictResize` (same logic as it used to have,
      but better name describing it)
       
      related to discussion
      https://github.com/redis/redis/pull/12819#discussion_r1409293878
      
      
      
      ---------
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      Co-authored-by: default avatarzhaozhao.zz <zhaozhao.zz@alibaba-inc.com>
      e2b7932b
    • zhaozhao.zz's avatar
      fix scripts access wrong slot if they disagree with pre-declared keys (#12906) · bb2b6e29
      zhaozhao.zz authored
      Regarding how to obtain the hash slot of a key, there is an optimization
      in `getKeySlot()`, it is used to avoid redundant hash calculations for
      keys: when the current client is in the process of executing a command,
      it can directly use the slot of the current client because the slot to
      access has already been calculated in advance in `processCommand()`.
      
      However, scripts are a special case where, in default mode or with
      `allow-cross-slot-keys` enabled, they are allowed to access keys beyond
      the pre-declared range. This means that the keys they operate on may not
      belong to the slot of the pre-declared keys. Currently, when the
      commands in a script are executed, the slot of the original client
      (i.e., the current client) is not correctly updated, leading to
      subsequent access to the wrong slot.
      
      This PR fixes the above issue. When checking the cluster constraints in
      a script, the slot to be accessed by the current command is set for the
      original client (i.e., the current client). This ensures that
      `getKeySlot()` gets the correct slot cache.
      
      Additionally, the following modifications are made:
      
      1. The 'sort' and 'sort_ro' commands use `getKeySlot()` instead of
      `c->slot` because the client could be an engine client in a script and
      can lead to potential bug.
      2. `getKeySlot()` is also used in pubsub to obtain the slot for the
      channel, standardizing the way slots are retrieved.
      bb2b6e29
  11. 14 Jan, 2024 1 commit
  12. 12 Jan, 2024 1 commit
    • Chen Tianjie's avatar
      Correct bytes_per_key computing. (#12897) · 87786342
      Chen Tianjie authored
      Change the calculation method of bytes_per_key to make it closer to
      the true average key size. The calculation method is as follows:
      
      mh->bytes_per_key = mh->total_keys ? (mh->dataset / mh->total_keys) : 0;
      87786342
  13. 11 Jan, 2024 2 commits
  14. 09 Jan, 2024 1 commit
    • Oran Agra's avatar
      Fix possible corruption in sdsResize (CVE-2023-41056) (#12924) · f7b1d028
      Oran Agra authored
      #11766 introduced a bug in sdsResize where it could forget to update the
      sds type in the sds header and then cause an overflow in sdsalloc. it
      looks like the only implication of that is a possible assertion in HLL,
      but it's hard to rule out possible heap corruption issues with
      clientsCronResizeQueryBuffer
      f7b1d028
  15. 08 Jan, 2024 5 commits
    • Binbin's avatar
      Fix minor fd leak in rdbSaveToSlavesSockets (#12919) · 14e4a983
      Binbin authored
      We should close server.rdb_child_exit_pipe when redisFork fails,
      otherwise the pipe fd will be leaked.
      
      Just a cleanup.
      14e4a983
    • Andy Pan's avatar
      Re-indent code and reduce code being complied on Solaris for anetKeepAlive (#12914) · 50b8b997
      Andy Pan authored
      This is a follow-up PR for #12782, in which we introduced nested
      preprocessor directives for TCP keep-alive on Solaris and added
      redundant indentation for code. Besides, it could result in unreachable
      code due to the lack of `#else` on the latest Solaris 11.4 where
      `TCP_KEEPIDLE`, `TCP_KEEPINTVL`, and `TCP_KEEPCNT` are available. As a
      result, this PR does three main things:
      
      - To eliminate the redundant indention for C code in nested preprocessor
      directives
      - To add `#else` directives and move `TCP_KEEPALIVE_THRESHOLD` +
      `TCP_KEEPALIVE_ABORT_THRESHOLD` settings under it, avoid unreachable
      code and compiler warnings when `#if defined(TCP_KEEPIDLE) &&
      defined(TCP_KEEPINTVL) && defined(TCP_KEEPCNT)` is met on Solaris 11.4
      - To remove a few trailing whitespace in comments
      50b8b997
    • Yanqi Lv's avatar
      Optimize performance when many clients [p|s]unsubscribe simultaneously (#12838) · c452e414
      Yanqi Lv authored
      I'm testing the performance of Pub/Sub command recently. I find if many
      clients unsubscribe or are killed simultaneously, Redis needs a long
      time to deal with it.
      
      In my experiment, I set 5000 clients and each client subscribes 100
      channels. Then I call `client kill type pubsub` to simulate the
      situation where clients unsubscribe all channels at the same time and
      calculate the execution time. The result shows that it takes about 23s.
      I use the _perf_ and find that `listSearchKey` in
      `pubsubUnsubscribeChannel` costs more than 90% cpu time. I think we can
      optimize this situation.
      
      In this PR, I replace list with dict to track the clients subscribing
      the channel more efficiently. It changes O(N) to O(1) in the search
      phase. Then I repeat the experiment as above. The results are as
      follows.
      
      |              | Execution Time(s) |used_memory(MB) |
      | :---------------- | :------: | :----: |
      | unstable(1bd0b549)        |   23.734   | 65.41 |
      | optimize-pubsub           |   0.288   | 67.66 |
      
      Thanks for #11595 , I use a no-value dict and the results shows that the
      performance improves significantly but the memory usage only increases
      slightly.
      
      Notice:
      
      - This PR will cause the performance degradation about 20% in
      `[p|s]subscribe` command but won't freeze Redis.
      c452e414
    • debing.sun's avatar
      Change destination key's key-spec flag from RW to OW for SINTERSTORE command (#12917) · 4730563e
      debing.sun authored
      In #10122, we set the destination key's flag of SINTERSTORE to `RW`, 
      however, this command doesn't actually read or modify the destination
      key, just overwrites it.
      Therefore, we change it to `OW` similarly to all other *STORE commands.
      4730563e
    • Binbin's avatar
      Fix CLUSTER SHARDS crash in 7.0/7.2 mixed clusters where shard ids are not sync (#12832) · 5b0c6a82
      Binbin authored
      Crash reported in #12695. In the process of upgrading the cluster from
      7.0 to 7.2, because the 7.0 nodes will not gossip shard id, in 7.2 we
      will rely on shard id to build the server.cluster->shards dict.
      
      In some cases, for example, the 7.0 master node and the 7.2 replica node.
      From the view of 7.2 replica node, the cluster->shards dictionary does not
      have its master node. In this case calling CLUSTER SHARDS on the 7.2 replica
      node may crash.
      
      We should fix the underlying assumption of updateShardId, which is that the
      shard dict should be always in sync with the node's shard_id. The fix was
      suggested by PingXie, see more details in #12695.
      5b0c6a82
  16. 07 Jan, 2024 1 commit
    • debing.sun's avatar
      Make RM_Yield thread-safe (#12905) · ca1f67af
      debing.sun authored
      ## Issues and solutions from #12817
      1. Touch ProcessingEventsWhileBlocked and calling moduleCount() without
      GIL in afterSleep()
          - Introduced: 
             Version: 7.0.0
             PR: #9963
      
         - Harm Level: Very High
      If the module thread calls `RM_Yield()` before the main thread enters
      afterSleep(),
      and modifies `ProcessingEventsWhileBlocked`(+1), it will cause the main
      thread to not wait for GIL,
      which can lead to all kinds of unforeseen problems, including memory
      data corruption.
      
         - Initial / Abandoned Solution:
            * Added `__thread` specifier for ProcessingEventsWhileBlocked.
      `ProcessingEventsWhileBlocked` is used to protect against nested event
      processing, but event processing
      in the main thread and module threads should be completely independent
      and unaffected, so it is safer
               to use TLS.
      * Adding a cached module count to keep track of the current number of
      modules, to avoid having to use `dictSize()`.
          
          - Related Warnings:
      ```
      WARNING: ThreadSanitizer: data race (pid=1136)
        Write of size 4 at 0x0001045990c0 by thread T4 (mutexes: write M0):
          #0 processEventsWhileBlocked networking.c:4135 (redis-server:arm64+0x10006d124)
          #1 RM_Yield module.c:2410 (redis-server:arm64+0x10018b66c)
          #2 bg_call_worker <null>:83232836 (blockedclient.so:arm64+0x16a8)
      
        Previous read of size 4 at 0x0001045990c0 by main thread:
          #0 afterSleep server.c:1861 (redis-server:arm64+0x100024f98)
          #1 aeProcessEvents ae.c:408 (redis-server:arm64+0x10000fd64)
          #2 aeMain ae.c:496 (redis-server:arm64+0x100010f0c)
          #3 main server.c:7220 (redis-server:arm64+0x10003f38c)
      ```
      
      2. aeApiPoll() is not thread-safe
      When using RM_Yield to handle events in a module thread, if the main
      thread has not yet
      entered `afterSleep()`, both the module thread and the main thread may
      touch `server.el` at the same time.
      
          - Introduced: 
             Version: 7.0.0
             PR: #9963
      
         - Old / Abandoned Solution:
      Adding a new mutex to protect timing between after beforeSleep() and
      before afterSleep().
      Defect: If the main thread enters the ae loop without any IO events, it
      will wait until
      the next timeout or until there is any event again, and the module
      thread will
      always hang until the main thread leaves the event loop.
      
          - Related Warnings:
      ```
      SUMMARY: ThreadSanitizer: data race ae_kqueue.c:55 in addEventMask
      ==================
      ==================
      WARNING: ThreadSanitizer: data race (pid=14682)
        Write of size 4 at 0x000100b54000 by thread T9 (mutexes: write M0):
          #0 aeApiPoll ae_kqueue.c:175 (redis-server:arm64+0x100010588)
          #1 aeProcessEvents ae.c:399 (redis-server:arm64+0x10000fb84)
          #2 processEventsWhileBlocked networking.c:4138 (redis-server:arm64+0x10006d3c4)
          #3 RM_Yield module.c:2410 (redis-server:arm64+0x10018b66c)
          #4 bg_call_worker <null>:16042052 (blockedclient.so:arm64+0x169c)
      
        Previous write of size 4 at 0x000100b54000 by main thread:
          #0 aeApiPoll ae_kqueue.c:175 (redis-server:arm64+0x100010588)
          #1 aeProcessEvents ae.c:399 (redis-server:arm64+0x10000fb84)
          #2 aeMain ae.c:496 (redis-server:arm64+0x100010da8)
          #3 main server.c:7238 (redis-server:arm64+0x10003f51c)
      ```
      
      ## The final fix as the comments:
      https://github.com/redis/redis/pull/12817#discussion_r1436427232
      Optimized solution based on the above comment:
      
      First, we add `module_gil_acquring` to indicate whether the main thread
      is currently in the acquiring GIL state.
      
      When the module thread starts to yield, there are two possibilities(we
      assume the caller keeps the GIL):
      1. The main thread is in the mid of beforeSleep() and afterSleep(), that
      is, `module_gil_acquring` is not 1 now.
      At this point, the module thread will wake up the main thread through
      the pipe and leave the yield,
      waiting for the next yield when the main thread may already in the
      acquiring GIL state.
          
      2. The main thread is in the acquiring GIL state.
      The module thread release the GIL, yielding CPU to give the main thread
      an opportunity to start
      event processing, and then acquire the GIL again until the main thread
      releases it.
      This is what
      https://github.com/redis/redis/pull/12817#discussion_r1436427232
      
      
      mentioned direction.
      
      ---------
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      ca1f67af