1. 06 Sep, 2024 1 commit
  2. 04 Sep, 2024 2 commits
    • debing.sun's avatar
      Introduce reusable query buffer for client reads (#13488) · ea3e8b79
      debing.sun authored
      This PR is based on the commits from PR
      https://github.com/valkey-io/valkey/pull/258,
      https://github.com/valkey-io/valkey/pull/593,
      https://github.com/valkey-io/valkey/pull/639
      
      This PR optimizes client query buffer handling in Redis by introducing
      a reusable query buffer that is used by default for client reads. This
      reduces memory usage by ~20KB per client by avoiding allocations for
      most clients using short (<16KB) complete commands. For larger or
      partial commands, the client still gets its own private buffer.
      
      The primary changes are:
      
      * Adding a reusable query buffer `thread_shared_qb` that clients use by
      default.
      * Modifying client querybuf initialization and reset logic.
      * Freeing idle client query buffers when empty to allow reuse of the
      reusable query buffer.
      * Master client query buffers are kept private as their contents need to
      be preserved for replication stream.
      * When nested commands is executed, only the first user uses the reuse
      buffer, and subsequent users will still use the private buffer.
      
      In addition to the memory savings, this change shows a 3% improvement in
      latency and throughput when running with 1000 active clients.
      
      The memory reduction may also help reduce the need to evict clients when
      reaching max memory limit, as the query buffer is the main memory
      consumer per client.
      
      This PR is different from https://github.com/valkey-io/valkey/pull/258
      
      
      1. When a client is in the mid of requiring a reused buffer and
      returning it, regardless of whether the query buffer has changed
      (expanded), we do not update the reused query buffer in the middle, but
      return the reused query buffer (expanded or with data remaining) or
      reset it at the end.
      2. Adding a new thread variable `thread_shared_qb_used` to avoid
      multiple clients requiring the reusable query buffer at the same time.
      
      ---------
      Signed-off-by: default avatarUri Yagelnik <uriy@amazon.com>
      Signed-off-by: default avatarMadelyn Olson <matolson@amazon.com>
      Co-authored-by: default avatarUri Yagelnik <uriy@amazon.com>
      Co-authored-by: default avatarMadelyn Olson <madelyneolson@gmail.com>
      Co-authored-by: default avataroranagra <oran@redislabs.com>
      ea3e8b79
    • Filipe Oliveira (Redis)'s avatar
      Optimize SET/INCR*/DECR*/SETRANGE/APPEND by reducing duplicate computation (#13505) · 05aed4ca
      Filipe Oliveira (Redis) authored
      
      
      - Avoid addReplyLongLong (which converts back to string) the value we
      already have as a robj, by using addReplyProto + addReply
      - Avoid doing dbFind Twice for the same dictEntry on
      INCR*/DECR*/SETRANGE/APPEND commands.
      - Avoid multiple sdslen calls with the same input on setrangeCommand and
      appendCommand
      - Introduce setKeyWithDictEntry, which is like setKey(), but accepts an
      optional dictEntry input: Avoids the second dictFind in SET command
      
      ---------
      Co-authored-by: default avatardebing.sun <debing.sun@redis.com>
      05aed4ca
  3. 03 Sep, 2024 1 commit
    • Filipe Oliveira (Redis)'s avatar
      Optimize the HELLO command reply (#13490) · a31b516e
      Filipe Oliveira (Redis) authored
      # Overall improvement
      
      TBD ( current is approximately 6% on the achievable ops/sec), coming
      from:
      
      - In case of no module we can skip 1.3% CPU cycles on dict Iterator
      creation/deletion
      - Use addReplyBulkCBuffer instead of addReplyBulkCString to avoid
      runtime strlen overhead within HELLO reply on string constants.
      
      ## Optimization 1: In case of no module we can skip 1.3% CPU cycles on
      dict Iterator creation/deletion.
      
      ## Optimization 2: Use addReplyBulkCBuffer instead of
      addReplyBulkCString to avoid runtime strlen overhead within HELLO reply
      on string constants.
      a31b516e
  4. 19 Aug, 2024 1 commit
    • Meir Shpilraien (Spielrein)'s avatar
      Avoid used_memory contention when update from multiple threads. (#13431) · 3264deb2
      Meir Shpilraien (Spielrein) authored
      The PR attempt to avoid contention on the `used_memory` global variable
      when allocate or free memory from multiple threads at the same time.
      
      Each time a thread is allocating or releasing a memory, it needs to
      update the `used_memory` global variable. This update might cause a
      contention when done aggressively from multiple threads.
      
      ### The solution
      
      Instead of having a single global variable that need to be updated from
      multiple thread. We create an array of used_memory, each entry in the
      array is updated by a single thread and the main thread summarizes all
      the values to accumulate the memory usage.
      
      This solution, though reduces the contention between threads on updating
      the `used_memory` global variable, it adds work to the main thread that
      need to summarize all the entries at the `used_memory` array. To avoid
      increasing the work done by the main thread by too much, we limit the
      size of the used memory array to 16. This means that up to 16 threads
      can run without any contention between them. If there are more than 16
      threads, we will reuse entries on the used_memory array, in this case we
      might still have contention between threads, but it will be much less
      significant.
      
      Notice, that in order to really avoid contention, the entries in the
      `used_memory` array must reside on different cache lines. To achieve
      that we create a struct with padding such that its size will be exactly
      cache_line size. In addition we make sure the address of the
      `used_memory` array will be aligned to cache_line size.
      
      ### Benchmark
      
      Some benchmark shows improvement (up to 15%):
      
      | Test Case |Baseline unstable (median obs. +- std.dev)|Comparison
      test_used_memory_per_thread_array (median obs. +- std.dev)|% change
      (higher-better)| Note |
      
      |-------------------------------------------------------------------------------|------------------------------------------|--------------------------------------------------------------------:|------------------------|------------------------------------|
      |memtier_benchmark-1key-list-100-elements-lrange-all-elements | 92657 +-
      2.0% (2 datapoints) | 101445|9.5% |IMPROVEMENT |
      |memtier_benchmark-1key-list-1K-elements-lrange-all-elements | 14965 +-
      1.3% (2 datapoints) | 16296|8.9% |IMPROVEMENT |
      |memtier_benchmark-1key-set-10-elements-smembers-pipeline-10 | 431019 +-
      5.2% (2 datapoints) | 461039|7.0% |waterline=5.2%. IMPROVEMENT |
      |memtier_benchmark-1key-set-100-elements-smembers | 74367 +- 0.0% (2
      datapoints) | 80190|7.8% |IMPROVEMENT |
      |memtier_benchmark-1key-set-1K-elements-smembers | 11730 +- 0.4% (2
      datapoints) | 13519|15.3% |IMPROVEMENT |
      
      
      Full results:
      
      | Test Case |Baseline unstable (median obs. +- std.dev)|Comparison
      test_used_memory_per_thread_array (median obs. +- std.dev)|% change
      (higher-better)| Note |
      
      |-------------------------------------------------------------------------------|------------------------------------------|--------------------------------------------------------------------:|------------------------|------------------------------------|
      |memtier_benchmark-10Mkeys-load-hash-5-fields-with-1000B-values | 88613
      +- 1.0% (2 datapoints) | 88688|0.1% |No Change |
      
      |memtier_benchmark-10Mkeys-load-hash-5-fields-with-1000B-values-pipeline-10
      | 124786 +- 1.2% (2 datapoints) | 123671|-0.9% |No Change |
      |memtier_benchmark-10Mkeys-load-hash-5-fields-with-100B-values | 122460
      +- 1.4% (2 datapoints) | 122990|0.4% |No Change |
      
      |memtier_benchmark-10Mkeys-load-hash-5-fields-with-100B-values-pipeline-10
      | 333384 +- 5.1% (2 datapoints) | 319221|-4.2% |waterline=5.1%.
      potential REGRESSION|
      |memtier_benchmark-10Mkeys-load-hash-5-fields-with-10B-values | 137354
      +- 0.3% (2 datapoints) | 138759|1.0% |No Change |
      
      |memtier_benchmark-10Mkeys-load-hash-5-fields-with-10B-values-pipeline-10
      | 401261 +- 4.3% (2 datapoints) | 398524|-0.7% |No Change |
      |memtier_benchmark-1Mkeys-100B-expire-use-case | 179058 +- 0.4% (2
      datapoints) | 180114|0.6% |No Change |
      |memtier_benchmark-1Mkeys-10B-expire-use-case | 180390 +- 0.2% (2
      datapoints) | 180401|0.0% |No Change |
      |memtier_benchmark-1Mkeys-1KiB-expire-use-case | 175993 +- 0.7% (2
      datapoints) | 175147|-0.5% |No Change |
      |memtier_benchmark-1Mkeys-4KiB-expire-use-case | 165771 +- 0.0% (2
      datapoints) | 164434|-0.8% |No Change |
      |memtier_benchmark-1Mkeys-bitmap-getbit-pipeline-10 | 931339 +- 2.1% (2
      datapoints) | 929487|-0.2% |No Change |
      |memtier_benchmark-1Mkeys-generic-exists-pipeline-10 | 999462 +- 0.4% (2
      datapoints) | 963226|-3.6% |potential REGRESSION |
      |memtier_benchmark-1Mkeys-generic-expire-pipeline-10 | 905333 +- 1.4% (2
      datapoints) | 896673|-1.0% |No Change |
      |memtier_benchmark-1Mkeys-generic-expireat-pipeline-10 | 885015 +- 1.0%
      (2 datapoints) | 865010|-2.3% |No Change |
      |memtier_benchmark-1Mkeys-generic-pexpire-pipeline-10 | 897115 +- 1.2%
      (2 datapoints) | 887544|-1.1% |No Change |
      |memtier_benchmark-1Mkeys-generic-scan-pipeline-10 | 451103 +- 3.2% (2
      datapoints) | 465571|3.2% |potential IMPROVEMENT |
      |memtier_benchmark-1Mkeys-generic-touch-pipeline-10 | 996809 +- 0.6% (2
      datapoints) | 984478|-1.2% |No Change |
      |memtier_benchmark-1Mkeys-generic-ttl-pipeline-10 | 979570 +- 1.7% (2
      datapoints) | 958752|-2.1% |No Change |
      
      |memtier_benchmark-1Mkeys-hash-hget-hgetall-hkeys-hvals-with-100B-values
      | 180888 +- 0.5% (2 datapoints) | 182295|0.8% |No Change |
      
      |memtier_benchmark-1Mkeys-hash-hmget-5-fields-with-100B-values-pipeline-10
      | 717881 +- 1.0% (2 datapoints) | 724814|1.0% |No Change |
      |memtier_benchmark-1Mkeys-hash-transactions-multi-exec-pipeline-20 |
      1055447 +- 0.4% (2 datapoints) | 1065836|1.0% |No Change |
      |memtier_benchmark-1Mkeys-lhash-hexists | 164332 +- 0.1% (2 datapoints)
      | 163636|-0.4% |No Change |
      |memtier_benchmark-1Mkeys-lhash-hincbry | 171674 +- 0.3% (2 datapoints)
      | 172737|0.6% |No Change |
      |memtier_benchmark-1Mkeys-list-lpop-rpop-with-100B-values | 180904 +-
      1.1% (2 datapoints) | 179467|-0.8% |No Change |
      |memtier_benchmark-1Mkeys-list-lpop-rpop-with-10B-values | 181746 +-
      0.8% (2 datapoints) | 182416|0.4% |No Change |
      |memtier_benchmark-1Mkeys-list-lpop-rpop-with-1KiB-values | 182004 +-
      0.7% (2 datapoints) | 180237|-1.0% |No Change |
      |memtier_benchmark-1Mkeys-load-hash-5-fields-with-1000B-values | 105191
      +- 0.9% (2 datapoints) | 105058|-0.1% |No Change |
      
      |memtier_benchmark-1Mkeys-load-hash-5-fields-with-1000B-values-pipeline-10
      | 150683 +- 0.9% (2 datapoints) | 153597|1.9% |No Change |
      |memtier_benchmark-1Mkeys-load-hash-hmset-5-fields-with-1000B-values |
      104122 +- 0.7% (2 datapoints) | 105236|1.1% |No Change |
      |memtier_benchmark-1Mkeys-load-list-with-100B-values | 149770 +- 0.9% (2
      datapoints) | 150510|0.5% |No Change |
      |memtier_benchmark-1Mkeys-load-list-with-10B-values | 165537 +- 1.9% (2
      datapoints) | 164329|-0.7% |No Change |
      |memtier_benchmark-1Mkeys-load-list-with-1KiB-values | 113315 +- 0.5% (2
      datapoints) | 114110|0.7% |No Change |
      |memtier_benchmark-1Mkeys-load-stream-1-fields-with-100B-values | 131201
      +- 0.7% (2 datapoints) | 129545|-1.3% |No Change |
      
      |memtier_benchmark-1Mkeys-load-stream-1-fields-with-100B-values-pipeline-10
      | 352891 +- 2.8% (2 datapoints) | 348338|-1.3% |No Change |
      |memtier_benchmark-1Mkeys-load-stream-5-fields-with-100B-values | 104386
      +- 0.7% (2 datapoints) | 105796|1.4% |No Change |
      
      |memtier_benchmark-1Mkeys-load-stream-5-fields-with-100B-values-pipeline-10
      | 227593 +- 5.5% (2 datapoints) | 218783|-3.9% |waterline=5.5%.
      potential REGRESSION|
      |memtier_benchmark-1Mkeys-load-string-with-100B-values | 167552 +- 0.2%
      (2 datapoints) | 170282|1.6% |No Change |
      |memtier_benchmark-1Mkeys-load-string-with-100B-values-pipeline-10 |
      646888 +- 0.5% (2 datapoints) | 639680|-1.1% |No Change |
      |memtier_benchmark-1Mkeys-load-string-with-10B-values | 174891 +- 0.7%
      (2 datapoints) | 174382|-0.3% |No Change |
      |memtier_benchmark-1Mkeys-load-string-with-10B-values-pipeline-10 |
      749988 +- 5.1% (2 datapoints) | 769986|2.7% |waterline=5.1%. No Change |
      |memtier_benchmark-1Mkeys-load-string-with-1KiB-values | 155929 +- 0.1%
      (2 datapoints) | 156387|0.3% |No Change |
      |memtier_benchmark-1Mkeys-load-zset-with-10-elements-double-score |
      92241 +- 0.2% (2 datapoints) | 92189|-0.1% |No Change |
      |memtier_benchmark-1Mkeys-load-zset-with-10-elements-int-score | 114328
      +- 1.3% (2 datapoints) | 113154|-1.0% |No Change |
      |memtier_benchmark-1Mkeys-string-get-100B | 180685 +- 0.2% (2
      datapoints) | 180359|-0.2% |No Change |
      |memtier_benchmark-1Mkeys-string-get-100B-pipeline-10 | 991291 +- 3.1%
      (2 datapoints) | 1020086|2.9% |No Change |
      |memtier_benchmark-1Mkeys-string-get-10B | 181183 +- 0.3% (2 datapoints)
      | 177868|-1.8% |No Change |
      |memtier_benchmark-1Mkeys-string-get-10B-pipeline-10 | 1032554 +- 0.8%
      (2 datapoints) | 1023120|-0.9% |No Change |
      |memtier_benchmark-1Mkeys-string-get-1KiB | 180479 +- 0.9% (2
      datapoints) | 182215|1.0% |No Change |
      |memtier_benchmark-1Mkeys-string-get-1KiB-pipeline-10 | 979286 +- 0.9%
      (2 datapoints) | 989888|1.1% |No Change |
      |memtier_benchmark-1Mkeys-string-mget-1KiB | 121950 +- 0.4% (2
      datapoints) | 120996|-0.8% |No Change |
      |memtier_benchmark-1key-geo-60M-elements-geodist | 179404 +- 1.0% (2
      datapoints) | 181232|1.0% |No Change |
      |memtier_benchmark-1key-geo-60M-elements-geodist-pipeline-10 | 1023797
      +- 0.5% (2 datapoints) | 1014980|-0.9% |No Change |
      |memtier_benchmark-1key-geo-60M-elements-geohash | 180808 +- 1.2% (2
      datapoints) | 180606|-0.1% |No Change |
      |memtier_benchmark-1key-geo-60M-elements-geohash-pipeline-10 | 1056458
      +- 1.6% (2 datapoints) | 1040050|-1.6% |No Change |
      |memtier_benchmark-1key-geo-60M-elements-geopos | 181808 +- 0.2% (2
      datapoints) | 175945|-3.2% |potential REGRESSION |
      |memtier_benchmark-1key-geo-60M-elements-geopos-pipeline-10 | 1038180 +-
      3.4% (2 datapoints) | 1033005|-0.5% |No Change |
      |memtier_benchmark-1key-geo-60M-elements-geosearch-fromlonlat | 142614
      +- 0.3% (2 datapoints) | 144259|1.2% |No Change |
      |memtier_benchmark-1key-geo-60M-elements-geosearch-fromlonlat-bybox |
      141008 +- 0.4% (2 datapoints) | 139602|-1.0% |No Change |
      
      |memtier_benchmark-1key-geo-60M-elements-geosearch-fromlonlat-pipeline-10
      | 560698 +- 0.8% (2 datapoints) | 548806|-2.1% |No Change |
      |memtier_benchmark-1key-list-10-elements-lrange-all-elements | 166132 +-
      0.9% (2 datapoints) | 170259|2.5% |No Change |
      |memtier_benchmark-1key-list-100-elements-lrange-all-elements | 92657 +-
      2.0% (2 datapoints) | 101445|9.5% |IMPROVEMENT |
      |memtier_benchmark-1key-list-1K-elements-lrange-all-elements | 14965 +-
      1.3% (2 datapoints) | 16296|8.9% |IMPROVEMENT |
      |memtier_benchmark-1key-pfadd-4KB-values-pipeline-10 | 264156 +- 0.2% (2
      datapoints) | 262582|-0.6% |No Change |
      |memtier_benchmark-1key-set-10-elements-smembers | 138916 +- 1.7% (2
      datapoints) | 138016|-0.6% |No Change |
      |memtier_benchmark-1key-set-10-elements-smembers-pipeline-10 | 431019 +-
      5.2% (2 datapoints) | 461039|7.0% |waterline=5.2%. IMPROVEMENT |
      |memtier_benchmark-1key-set-10-elements-smismember | 173545 +- 1.1% (2
      datapoints) | 173488|-0.0% |No Change |
      |memtier_benchmark-1key-set-100-elements-smembers | 74367 +- 0.0% (2
      datapoints) | 80190|7.8% |IMPROVEMENT |
      |memtier_benchmark-1key-set-100-elements-smismember | 155682 +- 1.6% (2
      datapoints) | 151367|-2.8% |No Change |
      |memtier_benchmark-1key-set-1K-elements-smembers | 11730 +- 0.4% (2
      datapoints) | 13519|15.3% |IMPROVEMENT |
      |memtier_benchmark-1key-set-200K-elements-sadd-constant | 181070 +- 1.1%
      (2 datapoints) | 180214|-0.5% |No Change |
      |memtier_benchmark-1key-set-2M-elements-sadd-increasing | 166364 +- 0.1%
      (2 datapoints) | 166944|0.3% |No Change |
      |memtier_benchmark-1key-zincrby-1M-elements-pipeline-1 | 46071 +- 0.6%
      (2 datapoints) | 44979|-2.4% |No Change |
      |memtier_benchmark-1key-zrank-1M-elements-pipeline-1 | 48429 +- 0.4% (2
      datapoints) | 49265|1.7% |No Change |
      |memtier_benchmark-1key-zrem-5M-elements-pipeline-1 | 48528 +- 0.4% (2
      datapoints) | 48869|0.7% |No Change |
      |memtier_benchmark-1key-zrevrangebyscore-256K-elements-pipeline-1 |
      100580 +- 1.5% (2 datapoints) | 101782|1.2% |No Change |
      |memtier_benchmark-1key-zrevrank-1M-elements-pipeline-1 | 48621 +- 2.0%
      (2 datapoints) | 48473|-0.3% |No Change |
      |memtier_benchmark-1key-zset-10-elements-zrange-all-elements | 83485 +-
      0.6% (2 datapoints) | 83095|-0.5% |No Change |
      
      |memtier_benchmark-1key-zset-10-elements-zrange-all-elements-long-scores
      | 118673 +- 0.8% (2 datapoints) | 118006|-0.6% |No Change |
      |memtier_benchmark-1key-zset-100-elements-zrange-all-elements | 19009 +-
      1.1% (2 datapoints) | 19293|1.5% |No Change |
      |memtier_benchmark-1key-zset-100-elements-zrangebyscore-all-elements |
      18957 +- 0.5% (2 datapoints) | 19419|2.4% |No Change |
      
      |memtier_benchmark-1key-zset-100-elements-zrangebyscore-all-elements-long-scores|
      171693 +- 0.5% (2 datapoints) | 172432|0.4% |No Change |
      |memtier_benchmark-1key-zset-1K-elements-zrange-all-elements | 3566 +-
      0.6% (2 datapoints) | 3672|3.0% |No Change |
      |memtier_benchmark-1key-zset-1M-elements-zcard-pipeline-10 | 1067713 +-
      0.4% (2 datapoints) | 1071550|0.4% |No Change |
      |memtier_benchmark-1key-zset-1M-elements-zrevrange-5-elements | 169195
      +- 0.7% (2 datapoints) | 169620|0.3% |No Change |
      |memtier_benchmark-1key-zset-1M-elements-zscore-pipeline-10 | 914338 +-
      0.2% (2 datapoints) | 905540|-1.0% |No Change |
      |memtier_benchmark-2keys-lua-eval-hset-expire | 88346 +- 1.7% (2
      datapoints) | 87259|-1.2% |No Change |
      |memtier_benchmark-2keys-lua-evalsha-hset-expire | 103273 +- 1.2% (2
      datapoints) | 102393|-0.9% |No Change |
      |memtier_benchmark-2keys-set-10-100-elements-sdiff | 15418 +- 10.9%
      UNSTABLE (2 datapoints) | 14369|-6.8% |UNSTABLE (very high variance) |
      |memtier_benchmark-2keys-set-10-100-elements-sinter | 83601 +- 3.6% (2
      datapoints) | 82508|-1.3% |No Change |
      |memtier_benchmark-2keys-set-10-100-elements-sunion | 14942 +- 11.2%
      UNSTABLE (2 datapoints) | 14001|-6.3% |UNSTABLE (very high variance) |
      |memtier_benchmark-2keys-stream-5-entries-xread-all-entries | 75938 +-
      0.4% (2 datapoints) | 76565|0.8% |No Change |
      |memtier_benchmark-2keys-stream-5-entries-xread-all-entries-pipeline-10
      | 120781 +- 1.1% (2 datapoints) | 119142|-1.4% |No Change |
      3264deb2
  5. 15 Jul, 2024 1 commit
  6. 26 Jun, 2024 1 commit
    • debing.sun's avatar
      Don't keep global replication buffer reference for replicas marked CLIENT_CLOSE_ASAP (#13363) · 52e12d8b
      debing.sun authored
      
      
      In certain situations, we might generate a large number of propagates
      (e.g., multi/exec, Lua script, or a single command generating tons of
      propagations) within an event loop.
      During the process of propagating to a replica, if the replica is
      disconnected(marked as CLIENT_CLOSE_ASAP) due to exceeding the output
      buffer limit, we should remove its reference to the global replication
      buffer to avoid the global replication buffer being unable to be
      properly trimmed due to being referenced.
      
      ---------
      Co-authored-by: default avataroranagra <oran@redislabs.com>
      52e12d8b
  7. 30 May, 2024 1 commit
    • Valentino Geron's avatar
      Free current client asynchronously after user permissions changes (#13274) · 50569a90
      Valentino Geron authored
      The crash happens when the user that triggers the permission changes
      should be affected (and should be disconnected eventually).
      
      To handle such a scenario, we should use the
      `CLIENT_CLOSE_AFTER_COMMAND` flag.
      
      This commit encapsulates all the places that should be handled in the
      same way in `deauthenticateAndCloseClient`
      
      Also:
      * bugfix: during the ACL LOAD we ignore clients that are marked as
      `CLIENT MASTER`
      50569a90
  8. 29 May, 2024 1 commit
    • Moti Cohen's avatar
      HFE to support AOF and replicas (#13285) · 33fc0fbf
      Moti Cohen authored
      * For replica sake, rewrite commands `H*EXPIRE*` , `HSETF`, `HGETF` to
      have absolute unix time in msec.
      * On active-expiration of field, propagate HDEL to replica
      (`propagateHashFieldDeletion()`)
      * On lazy-expiration, propagate HDEL to replica (`hashTypeGetValue()`
      now calls `hashTypeDelete()`. It also takes care to call
      `propagateHashFieldDeletion()`).
      * Fix `H*EXPIRE*` command such that if it gets flag `LT` and it doesn’t
      have any expiration on the field then it will considered as valid
      condition.
      
      Note, replicas doesn’t make any active expiration, and should avoid lazy
      expiration. On `hashTypeGetValue()` it doesn't check expiration (As long
      as the master didn’t request to delete the field, it is valid)
      
      TODO: 
      * Attach `dbid` to HASH metadata. See
      [here](https://github.com/redis/redis/pull/13209#discussion_r1593385850
      
      )
      
      ---------
      Co-authored-by: default avatardebing.sun <debing.sun@redis.com>
      33fc0fbf
  9. 18 Apr, 2024 1 commit
    • Moti Cohen's avatar
      Hash Field Expiration - Basic support · c18ff056
      Moti Cohen authored
      - Add ebuckets & mstr data structures
      - Integrate active & lazy expiration
      - Add most of the commands 
      - Add support for dict (listpack is missing)
      TODOs:  RDB, notification, listpack, HSET, HGETF, defrag, aof
      c18ff056
  10. 20 Mar, 2024 1 commit
  11. 20 Feb, 2024 1 commit
    • Binbin's avatar
      Fix wathced client test timing issue caused by late close (#13062) · 3c2ea1ea
      Binbin authored
      There is a timing issue in the test, close may arrive late, or in
      freeClientAsync we will free the client in async way, which will
      lead to errors in watching_clients statistics, since we will only
      unwatch all keys when we truly freeClient.
      
      Add a wait here to avoid this problem. Also fixed some outdated
      comments i saw. The test was introduced in #12966.
      3c2ea1ea
  12. 18 Feb, 2024 1 commit
    • zhaozhao.zz's avatar
      Add metrics for WATCH (#12966) · 50d6fe8c
      zhaozhao.zz authored
      Redis has some special commands that mark the client's state, such as
      `subscribe` and `blpop`, which mark the client as `CLIENT_PUBSUB` or
      `CLIENT_BLOCKED`, and we have metrics for the special use cases.
      
      However, there are also other special commands, like `WATCH`, which
      although do not have a specific flags, and should also be considered
      stateful client types. For stateful clients, in many scenarios, the
      connections cannot be shared in "connection pool", meaning connection
      pool cannot be used. For example, whenever the `WATCH` command is
      executed, a new connection is required to put the client into the "watch
      state" because the watched keys are stored in the client.
      
      If different business logic requires watching different keys, separate
      connections must be used; otherwise, there will be contamination. This
      also means that if a user's business heavily relies on the `WATCH`
      command, a large number of connections will be required.
      
      Recently we have encountered this situation in our platform, where some
      users consume a significant number of connections when using Redis
      because of `WATCH`.
      
      I hope we can have a way to observe these special use cases and special
      client connections. Here I add a few monitoring metrics:
      
      1. `watching_clients` in `INFO` reply: The number of clients currently
      in the "watching" state.
      2. `total_watched_keys` in `INFO` reply: The total number of keys being
      watched.
      3. `watch` in `CLIENT LIST` reply: The number of keys each client is
      currently watching.
      50d6fe8c
  13. 30 Jan, 2024 1 commit
  14. 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
  15. 19 Jan, 2024 1 commit
    • 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
  16. 14 Dec, 2023 1 commit
    • Guillaume Koenig's avatar
      Extend rax usage by allowing any long long value (#12837) · 967fb3c6
      Guillaume Koenig authored
      The raxFind implementation uses a special pointer value (the address of
      a static string) as the "not found" value. It works as long as actual
      pointers were used. However we've seen usages where long long,
      non-pointer values have been used. It creates a risk that one of the
      long long value precisely is the address of the special "not found"
      value. This commit changes raxFind to return 1 or 0 to indicate
      elementhood, and take in a new void **value to optionally return the
      associated value.
      
      By extension, this also allow the RedisModule_DictSet/Replace operations
      to also safely insert integers instead of just pointers.
      967fb3c6
  17. 13 Dec, 2023 1 commit
  18. 22 Nov, 2023 1 commit
  19. 01 Nov, 2023 1 commit
  20. 28 Sep, 2023 1 commit
  21. 30 Aug, 2023 1 commit
    • Chen Tianjie's avatar
      Add two stats to count client input and output buffer oom. (#12476) · e3d4b30d
      Chen Tianjie authored
      Add these INFO metrics:
      * client_query_buffer_limit_disconnections
      * client_output_buffer_limit_disconnections
      
      Sometimes it is useful to monitor whether clients reaches size limit of
      query buffer and output buffer, to decide whether we need to adjust the
      buffer size limit or reduce client query payload.
      e3d4b30d
  22. 25 Jul, 2023 1 commit
    • zhaozhao.zz's avatar
      update monitor client's memory and evict correctly (#12420) · 01eb939a
      zhaozhao.zz authored
      A bug introduced in #11657 (7.2 RC1), causes client-eviction (#8687)
      and INFO to have inaccurate memory usage metrics of MONITOR clients.
      
      Because the type in `c->type` and the type in `getClientType()` are confusing
      (in the later, `CLIENT_TYPE_NORMAL` not `CLIENT_TYPE_SLAVE`), the comment
      we wrote in `updateClientMemUsageAndBucket` was wrong, and in fact that function
      didn't skip monitor clients.
      And since it doesn't skip monitor clients, it was wrong to delete the call for it from
      `replicationFeedMonitors` (it wasn't a NOP).
      That deletion could mean that the monitor client memory usage is not always up to
      date (updated less frequently, but still a candidate for client eviction).
      01eb939a
  23. 20 Jun, 2023 1 commit
    • Oran Agra's avatar
      Fix broken protocol when PUBLISH emits local push inside MULTI (#12326) · 8ad8f0f9
      Oran Agra authored
      When a connection that's subscribe to a channel emits PUBLISH inside MULTI-EXEC,
      the push notification messes up the EXEC response.
      
      e.g. MULTI, PING, PUSH foo bar, PING, EXEC
      the EXEC's response will contain: PONG, {message foo bar}, 1. and the second PONG
      will be delivered outside the EXEC's response.
      
      Additionally, this PR changes the order of responses in case of a plain PUBLISH (when
      the current client also subscribed to it), by delivering the push after the command's
      response instead of before it.
      This also affects modules calling RM_PublishMessage in a similar way, so that we don't
      run the risk of getting that push mixed together with the module command's response.
      8ad8f0f9
  24. 19 Jun, 2023 1 commit
    • Binbin's avatar
      Optimize PSUBSCRIBE and PUNSUBSCRIBE from O(N*M) to O(N) (#12298) · b5106249
      Binbin authored
      In the original implementation, the time complexity of the commands
      is actually O(N*M), where N is the number of patterns the client is
      already subscribed and M is the number of patterns to subscribe to.
      The docs are all wrong about this.
      
      Specifically, because the original client->pubsub_patterns is a list,
      so we need to do listSearchKey which is O(N). In this PR, we change it
      to a dict, so the search becomes O(1).
      
      At the same time, both pubsub_channels and pubsubshard_channels are dicts.
      Changing pubsub_patterns to a dictionary improves the readability and
      maintainability of the code.
      b5106249
  25. 12 Jun, 2023 1 commit
  26. 28 May, 2023 1 commit
  27. 03 May, 2023 1 commit
    • Madelyn Olson's avatar
      Remove prototypes with empty declarations (#12020) · 5e3be1be
      Madelyn Olson authored
      Technically declaring a prototype with an empty declaration has been deprecated since the early days of C, but we never got a warning for it. C2x will apparently be introducing a breaking change if you are using this type of declarator, so Clang 15 has started issuing a warning with -pedantic. Although not apparently a problem for any of the compiler we build on, if feels like the right thing is to properly adhere to the C standard and use (void).
      5e3be1be
  28. 18 Apr, 2023 1 commit
    • sundb's avatar
      Fix some compile warnings and errors when building with gcc-12 or clang (#12035) · 42c8c618
      sundb authored
      This PR is to fix the compilation warnings and errors generated by the latest
      complier toolchain, and to add a new runner of the latest toolchain for daily CI.
      
      ## Fix various compilation warnings and errors
      
      1) jemalloc.c
      
      COMPILER: clang-14 with FORTIFY_SOURCE
      
      WARNING:
      ```
      src/jemalloc.c:1028:7: warning: suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma? [-Wstring-concatenation]
                          "/etc/malloc.conf",
                          ^
      src/jemalloc.c:1027:3: note: place parentheses around the string literal to silence warning
                      "\"name\" of the file referenced by the symbolic link named "
                      ^
      ```
      
      REASON:  the compiler to alert developers to potential issues with string concatenation
      that may miss a comma,
      just like #9534 which misses a comma.
      
      SOLUTION: use `()` to tell the compiler that these two line strings are continuous.
      
      2) config.h
      
      COMPILER: clang-14 with FORTIFY_SOURCE
      
      WARNING:
      ```
      In file included from quicklist.c:36:
      ./config.h:319:76: warning: attribute declaration must precede definition [-Wignored-attributes]
      char *strcat(char *restrict dest, const char *restrict src) __attribute__((deprecated("please avoid use of unsafe C functions. prefer use of redis_strlcat instead")));
      ```
      
      REASON: Enabling _FORTIFY_SOURCE will cause the compiler to use `strcpy()` with check,
      it results in a deprecated attribute declaration after including <features.h>.
      
      SOLUTION: move the deprecated attribute declaration from config.h to fmacro.h before "#include <features.h>".
      
      3) networking.c
      
      COMPILER: GCC-12
      
      WARNING: 
      ```
      networking.c: In function ‘addReplyDouble.part.0’:
      networking.c:876:21: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
        876 |         dbuf[start] = '$';
            |                     ^
      networking.c:868:14: note: at offset -5 into destination object ‘dbuf’ of size 5152
        868 |         char dbuf[MAX_LONG_DOUBLE_CHARS+32];
            |              ^
      networking.c:876:21: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
        876 |         dbuf[start] = '$';
            |                     ^
      networking.c:868:14: note: at offset -6 into destination object ‘dbuf’ of size 5152
        868 |         char dbuf[MAX_LONG_DOUBLE_CHARS+32];
      ```
      
      REASON: GCC-12 predicts that digits10() may return 9 or 10 through `return 9 + (v >= 1000000000UL)`.
      
      SOLUTION: add an assert to let the compiler know the possible length;
      
      4) redis-cli.c & redis-benchmark.c
      
      COMPILER: clang-14 with FORTIFY_SOURCE
      
      WARNING:
      ```
      redis-benchmark.c:1621:2: warning: embedding a directive within macro arguments has undefined behavior [-Wembedded-directive] #ifdef USE_OPENSSL
      redis-cli.c:3015:2: warning: embedding a directive within macro arguments has undefined behavior [-Wembedded-directive] #ifdef USE_OPENSSL
      ```
      
      REASON: when _FORTIFY_SOURCE is enabled, the compiler will use the print() with
      check, which is a macro. this may result in the use of directives within the macro, which
      is undefined behavior.
      
      SOLUTION: move the directives-related code out of `print()`.
      
      5) server.c
      
      COMPILER: gcc-13 with FORTIFY_SOURCE
      
      WARNING:
      ```
      In function 'lookupCommandLogic',
          inlined from 'lookupCommandBySdsLogic' at server.c:3139:32:
      server.c:3102:66: error: '*(robj **)argv' may be used uninitialized [-Werror=maybe-uninitialized]
       3102 |     struct redisCommand *base_cmd = dictFetchValue(commands, argv[0]->ptr);
            |                                                              ~~~~^~~
      ```
      
      REASON: The compiler thinks that the `argc` returned by `sdssplitlen()` could be 0,
      resulting in an empty array of size 0 being passed to lookupCommandLogic.
      this should be a false positive, `argc` can't be 0 when strings are not NULL.
      
      SOLUTION: add an assert to let the compiler know that `argc` is positive.
      
      6) sha1.c
      
      COMPILER: gcc-12
      
      WARNING:
      ```
      In function ‘SHA1Update’,
          inlined from ‘SHA1Final’ at sha1.c:195:5:
      sha1.c:152:13: warning: ‘SHA1Transform’ reading 64 bytes from a region of size 0 [-Wstringop-overread]
        152 |             SHA1Transform(context->state, &data[i]);
            |             ^
      sha1.c:152:13: note: referencing argument 2 of type ‘const unsigned char[64]’
      sha1.c: In function ‘SHA1Final’:
      sha1.c:56:6: note: in a call to function ‘SHA1Transform’
         56 | void SHA1Transform(uint32_t state[5], const unsigned char buffer[64])
            |      ^
      In function ‘SHA1Update’,
          inlined from ‘SHA1Final’ at sha1.c:198:9:
      sha1.c:152:13: warning: ‘SHA1Transform’ reading 64 bytes from a region of size 0 [-Wstringop-overread]
        152 |             SHA1Transform(context->state, &data[i]);
            |             ^
      sha1.c:152:13: note: referencing argument 2 of type ‘const unsigned char[64]’
      sha1.c: In function ‘SHA1Final’:
      sha1.c:56:6: note: in a call to function ‘SHA1Transform’
         56 | void SHA1Transform(uint32_t state[5], const unsigned char buffer[64])
      ```
      
      REASON: due to the bug[https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80922], when
      enable LTO, gcc-12 will not see `diagnostic ignored "-Wstringop-overread"`, resulting in a warning.
      
      SOLUTION: temporarily set SHA1Update to noinline to avoid compiler warnings due
      to LTO being enabled until the above gcc bug is fixed.
      
      7) zmalloc.h
      
      COMPILER: GCC-12
      
      WARNING: 
      ```
      In function ‘memset’,
          inlined from ‘moduleCreateContext’ at module.c:877:5,
          inlined from ‘RM_GetDetachedThreadSafeContext’ at module.c:8410:5:
      /usr/include/x86_64-linux-gnu/bits/string_fortified.h:59:10: warning: ‘__builtin_memset’ writing 104 bytes into a region of size 0 overflows the destination [-Wstringop-overflow=]
         59 |   return __builtin___memset_chk (__dest, __ch, __len,
      ```
      
      REASON: due to the GCC-12 bug [https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96503],
      GCC-12 cannot see alloc_size, which causes GCC to think that the actual size of memory
      is 0 when checking with __glibc_objsize0().
      
      SOLUTION: temporarily set malloc-related interfaces to `noinline` to avoid compiler warnings
      due to LTO being enabled until the above gcc bug is fixed.
      
      ## Other changes
      1) Fixed `ps -p [pid]`  doesn't output `<defunct>` when using procps 4.x causing `replication
        child dies when parent is killed - diskless` test to fail.
      2) Add a new fortify CI with GCC-13 and ubuntu-lunar docker image.
      42c8c618
  29. 16 Apr, 2023 1 commit
    • judeng's avatar
      avoid incorrect shrinking of querybuf when client is reading a big argv (#12000) · e7f18432
      judeng authored
      this pr fix two wrongs:
      1. When client’s querybuf is pre-allocated for a fat argv, we need to update the
        querybuf_peak of the client immediately to completely avoid the unexpected
        shrinking of querybuf in the next clientCron (before data arrives to set the peak).
      2. the protocol's bulklen does not include `\r\n`, but the allocation and the data we
        read does. so in `clientsCronResizeQueryBuffer`, the `resize` or `querybuf_peak`
        should add these 2 bytes.
      
      the first bug is likely to hit us on large payloads over slow connections, in which case
      transferring the payload can take longer and a cron event will be triggered (specifically
      if there are not a lot of clients)
      e7f18432
  30. 13 Apr, 2023 1 commit
  31. 12 Apr, 2023 1 commit
    • Binbin's avatar
      Add RM_ReplyWithErrorFormat that can support format (#11923) · bfec2d70
      Binbin authored
      * Add RM_ReplyWithErrorFormat that can support format
      
      Reply with the error create from a printf format and arguments.
      
      If the error code is already passed in the string 'fmt', the error
      code provided is used, otherwise the string "-ERR " for the generic
      error code is automatically added.
      
      The usage is, for example:
          RedisModule_ReplyWithErrorFormat(ctx, "An error: %s", "foo");
          RedisModule_ReplyWithErrorFormat(ctx, "-WRONGTYPE Wrong Type: %s", "foo");
      
      The function always returns REDISMODULE_OK.
      bfec2d70
  32. 10 Apr, 2023 1 commit
    • sundb's avatar
      Use dummy allocator to make accesses defined as per standard (#11982) · e0b378d2
      sundb authored
      
      
      ## Issue
      When we use GCC-12 later or clang 9.0 later to build with `-D_FORTIFY_SOURCE=3`,
      we can see the following buffer overflow:
      ```
      === REDIS BUG REPORT START: Cut & paste starting from here ===
      6263:M 06 Apr 2023 08:59:12.915 # Redis 255.255.255 crashed by signal: 6, si_code: -6
      6263:M 06 Apr 2023 08:59:12.915 # Crashed running the instruction at: 0x7f03d59efa7c
      
      ------ STACK TRACE ------
      EIP:
      /lib/x86_64-linux-gnu/libc.so.6(pthread_kill+0x12c)[0x7f03d59efa7c]
      
      Backtrace:
      /lib/x86_64-linux-gnu/libc.so.6(+0x42520)[0x7f03d599b520]
      /lib/x86_64-linux-gnu/libc.so.6(pthread_kill+0x12c)[0x7f03d59efa7c]
      /lib/x86_64-linux-gnu/libc.so.6(raise+0x16)[0x7f03d599b476]
      /lib/x86_64-linux-gnu/libc.so.6(abort+0xd3)[0x7f03d59817f3]
      /lib/x86_64-linux-gnu/libc.so.6(+0x896f6)[0x7f03d59e26f6]
      /lib/x86_64-linux-gnu/libc.so.6(__fortify_fail+0x2a)[0x7f03d5a8f76a]
      /lib/x86_64-linux-gnu/libc.so.6(+0x1350c6)[0x7f03d5a8e0c6]
      src/redis-server 127.0.0.1:25111(+0xd5e80)[0x557cddd3be80]
      src/redis-server 127.0.0.1:25111(feedReplicationBufferWithObject+0x78)[0x557cddd3c768]
      src/redis-server 127.0.0.1:25111(replicationFeedSlaves+0x1a4)[0x557cddd3cbc4]
      src/redis-server 127.0.0.1:25111(+0x8721a)[0x557cddced21a]
      src/redis-server 127.0.0.1:25111(call+0x47a)[0x557cddcf38ea]
      src/redis-server 127.0.0.1:25111(processCommand+0xbf4)[0x557cddcf4aa4]
      src/redis-server 127.0.0.1:25111(processInputBuffer+0xe6)[0x557cddd22216]
      src/redis-server 127.0.0.1:25111(readQueryFromClient+0x3a8)[0x557cddd22898]
      src/redis-server 127.0.0.1:25111(+0x1b9134)[0x557cdde1f134]
      src/redis-server 127.0.0.1:25111(aeMain+0x119)[0x557cddce5349]
      src/redis-server 127.0.0.1:25111(main+0x466)[0x557cddcd6716]
      /lib/x86_64-linux-gnu/libc.so.6(+0x29d90)[0x7f03d5982d90]
      /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x80)[0x7f03d5982e40]
      src/redis-server 127.0.0.1:25111(_start+0x25)[0x557cddcd7025]
      ```
      
      The main reason is that when FORTIFY_SOURCE is enabled, GCC or clang will enhance some
      common functions, such as `strcpy`, `memcpy`, `fgets`, etc, so that they can detect buffer
      overflow errors and stop program execution, thus improving the safety of the program.
      We use `zmalloc_usable_size()` everywhere to use memory blocks, but that is an abuse since the
      malloc_usable_size() isn't meant for this kind of use, it is for diagnostics only. That is also why the
      behavior is flaky when built with _FORTIFY_SOURCE, the compiler can sense that we reach outside
      the allocated block and SIGABRT.
      
      ### Solution
      If we need to use the additional memory we got, we need to use a dummy realloc with `alloc_size` attribute
      and no inlining, (see `extend_to_usable`) to let the compiler see the large of memory we need to use.
      This can either be an implicit call inside `z*usable` that returns the size, so that the caller doesn't have any
      other worry, or it can be a normal zmalloc call which means that if the caller wants to use
      zmalloc_usable_size it must also use extend_to_usable.
      
      ### Changes
      
      This PR does the following:
      1) rename the current z[try]malloc_usable family to z[try]malloc_internal and don't expose them to users outside zmalloc.c,
      2) expose a new set of `z[*]_usable` family that use z[*]_internal and `extend_to_usable()` implicitly, the caller gets the
        size of the allocation and it is safe to use.
      3) go over all the users of `zmalloc_usable_size` and convert them to use the `z[*]_usable` family if possible.
      4) in the places where the caller can't use `z[*]_usable` and store the real size, and must still rely on zmalloc_usable_size,
        we still make sure that the allocation used `z[*]_usable` (which has a call to `extend_to_usable()`) and ignores the
        returning size, this way a later call to `zmalloc_usable_size` is still safe.
      
      [4] was done for module.c and listpack.c, all the others places (sds, reply proto list, replication backlog, client->buf)
      are using [3].
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      e0b378d2
  33. 04 Apr, 2023 1 commit
  34. 30 Mar, 2023 1 commit
    • Madelyn Olson's avatar
      Fixed tracking of command duration for multi/eval/module/wait (#11970) · 971b177f
      Madelyn Olson authored
      In #11012, we changed the way command durations were computed to handle the same command being executed multiple times. This commit fixes some misses from that commit.
      
      * Wait commands were not correctly reporting their duration if the timeout was reached.
      * Multi/scripts/and modules with RM_Call were not properly resetting the duration between inner calls, leading to them reporting cumulative duration.
      * When a blocked client is freed, the call and duration are always discarded.
      
      This commit also adds an assert if the duration is not properly reset, potentially indicating that a report to call statistics was missed. The assert potentially be removed in the future, as it's mainly intended to detect misses in tests.
      971b177f
  35. 22 Mar, 2023 3 commits
    • Oran Agra's avatar
      Fix reply schema validator with RESET command (#11953) · 3c4def56
      Oran Agra authored
      The reply schema validator is failing since the recent changes to introspection.tcl that use the RESET command, this happens because this test forces RESP3, but RESET command didn't respect that and set back RESP2.
      3c4def56
    • Oran Agra's avatar
    • Igor Malinovskiy's avatar
      Allow clients to report name and version (#11758) · c3b9f2fb
      Igor Malinovskiy authored
      
      
      This PR allows clients to send information about the client library to redis
      to be displayed in CLIENT LIST and CLIENT INFO.
      
      Currently supports:
      `CLIENT [lib-name | lib-ver] <value>`
      Client libraries are expected to pipeline these right after AUTH, and ignore
      the failure in case they're talking to an older version of redis.
      
      These will be shown in CLIENT LIST and CLIENT INFO as:
      * `lib-name` - meant to hold the client library name.
      * `lib-ver` - meant to hold the client library version.
      
      The values cannot contain spaces, newlines and any wild ASCII characters,
      but all other normal chars are accepted, e.g `.`, `=` etc (same as CLIENT NAME).
      
      The RESET command does NOT clear these, but they can be cleared to the
      default by sending a command with a blank string.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      c3b9f2fb
  36. 16 Mar, 2023 1 commit
    • Meir Shpilraien (Spielrein)'s avatar
      Support for RM_Call on blocking commands (#11568) · d0da0a6a
      Meir Shpilraien (Spielrein) authored
      Allow running blocking commands from within a module using `RM_Call`.
      
      Today, when `RM_Call` is used, the fake client that is used to run command
      is marked with `CLIENT_DENY_BLOCKING` flag. This flag tells the command
      that it is not allowed to block the client and in case it needs to block, it must
      fallback to some alternative (either return error or perform some default behavior).
      For example, `BLPOP` fallback to simple `LPOP` if it is not allowed to block.
      
      All the commands must respect the `CLIENT_DENY_BLOCKING` flag (including
      module commands). When the command invocation finished, Redis asserts that
      the client was not blocked.
      
      This PR introduces the ability to call blocking command using `RM_Call` by
      passing a callback that will be called when the client will get unblocked.
      In order to do that, the user must explicitly say that he allow to perform blocking
      command by passing a new format specifier argument, `K`, to the `RM_Call`
      function. This new flag will tell Redis that it is allow to run blocking command
      and block the client. In case the command got blocked, Redis will return a new
      type of call reply (`REDISMODULE_REPLY_PROMISE`). This call reply indicates
      that the command got blocked and the user can set the on_unblocked handler using
      `RM_CallReplyPromiseSetUnblockHandler`.
      
      When clients gets unblocked, it eventually reaches `processUnblockedClients` function.
      This is where we check if the client is a fake module client and if it is, we call the unblock
      callback instead of performing the usual unblock operations.
      
      **Notice**: `RM_CallReplyPromiseSetUnblockHandler` must be called atomically
      along side the command invocation (without releasing the Redis lock in between).
      In addition, unlike other CallReply types, the promise call reply must be released
      by the module when the Redis GIL is acquired.
      
      The module can abort the execution on the blocking command (if it was not yet
      executed) using `RM_CallReplyPromiseAbort`. the API will return `REDISMODULE_OK`
      on success and `REDISMODULE_ERR` if the operation is already executed.
      **Notice** that in case of misbehave module, Abort might finished successfully but the
      operation will not really be aborted. This can only happened if the module do not respect
      the disconnect callback of the blocked client. 
      For pure Redis commands this can not happened.
      
      ### Atomicity Guarantees
      
      The API promise that the unblock handler will run atomically as an execution unit.
      This means that all the operation performed on the unblock handler will be wrapped
      with a multi exec transaction when replicated to the replica and AOF.
      The API **do not** grantee any other atomicity properties such as when the unblock
      handler will be called. This gives us the flexibility to strengthen the grantees (or not)
      in the future if we will decide that we need a better guarantees.
      
      That said, the implementation **does** provide a better guarantees when performing
      pure Redis blocking command like `BLPOP`. In this case the unblock handler will run
      atomically with the operation that got unblocked (for example, in case of `BLPOP`, the
      unblock handler will run atomically with the `LPOP` operation that run when the command
      got unblocked). This is an implementation detail that might be change in the future and the
      module writer should not count on that.
      
      ### Calling blocking commands while running on script mode (`S`)
      
      `RM_Call` script mode (`S`) was introduced on #0372. It is used for usecases where the
      command that was invoked on `RM_Call` comes from a user input and we want to make
      sure the user will not run dangerous commands like `shutdown`. Some command, such
      as `BLPOP`, are marked with `NO_SCRIPT` flag, which means they will not be allowed on
      script mode. Those commands are marked with  `NO_SCRIPT` just because they are
      blocking commands and not because they are dangerous. Now that we can run blocking
      commands on RM_Call, there is no real reason not to allow such commands on script mode.
      
      The underline problem is that the `NO_SCRIPT` flag is abused to also mark some of the
      blocking commands (notice that those commands know not to block the client if it is not
      allowed to do so, and have a fallback logic to such cases. So even if those commands
      were not marked with `NO_SCRIPT` flag, it would not harm Redis, and today we can
      already run those commands within multi exec).
      
      In addition, not all blocking commands are marked with `NO_SCRIPT` flag, for example
      `blmpop` are not marked and can run from within a script.
      
      Those facts shows that there are some ambiguity about the meaning of the `NO_SCRIPT`
      flag, and its not fully clear where it should be use.
      
      The PR suggest that blocking commands should not be marked with `NO_SCRIPT` flag,
      those commands should handle `CLIENT_DENY_BLOCKING` flag and only block when
      it's safe (like they already does today). To achieve that, the PR removes the `NO_SCRIPT`
      flag from the following commands:
      * `blmove`
      * `blpop`
      * `brpop`
      * `brpoplpush`
      * `bzpopmax`
      * `bzpopmin`
      * `wait`
      
      This might be considered a breaking change as now, on scripts, instead of getting
      `command is not allowed from script` error, the user will get some fallback behavior
      base on the command implementation. That said, the change matches the behavior
      of scripts and multi exec with respect to those commands and allow running them on
      `RM_Call` even when script mode is used.
      
      ### Additional RedisModule API and changes
      
      * `RM_BlockClientSetPrivateData` - Set private data on the blocked client without the
        need to unblock the client. This allows up to set the promise CallReply as the private
        data of the blocked client and abort it if the client gets disconnected.
      * `RM_BlockClientGetPrivateData` - Return the current private data set on a blocked client.
        We need it so we will have access to this private data on the disconnect callback.
      * On RM_Call, the returned reply will be added to the auto memory context only if auto
        memory is enabled, this allows us to keep the call reply for longer time then the context
        lifetime and does not force an unneeded borrow relationship between the CallReply and
        the RedisModuleContext.
      d0da0a6a