1. 24 Apr, 2022 4 commits
    • guybe7's avatar
      isSafeToPerformEvictions: Remove redundant condition (#10610) · 8ad0cfa5
      guybe7 authored
      If was first added in #9890 to solve the problem of
      CONFIG SET maxmemory causing eviction inside MULTI/EXEC,
      but that problem is already fixed (CONFIG SET doesn't evict
      directly, it just schedules a later eviction)
      
      Keep that condition may hide bugs (i.e. performEvictions
      should always expect to have an empty server.also_propagate)
      8ad0cfa5
    • Binbin's avatar
      Fix timing issue in slowlog redact test (#10614) · a6b3ce28
      Binbin authored
      * Fix timing issue in slowlog redact test
      
      This test failed once in my daily CI (test-sanitizer-address (clang))
      ```
      *** [err]: SLOWLOG - Some commands can redact sensitive fields in tests/unit/slowlog.tcl
      Expected 'migrate 127.0.0.1 25649 key 9 5000 AUTH2 (redacted) (redacted)' to match '* key 9 5000 AUTH (redacted)' (context: type eval line 12 cmd {assert_match {* key 9 5000 AUTH (redacted)} [lindex [lindex [r slowlog get] 1] 3]} proc ::test)
      ```
      
      The reason is that with slowlog-log-slower-than 10000,
      slowlog get will have a chance to exceed 10ms.
      
      Change slowlog-log-slower-than from 10000 to -1, disable it.
      Also handles a same potentially problematic test above.
      This is actually the same timing issue as #10432.
      
      But also avoid repeated calls to `SLOWLOG GET`
      a6b3ce28
    • Lu JJ's avatar
    • Lu JJ's avatar
      fix typo in "lcsCommand" doc comment (#10622) · 719b7665
      Lu JJ authored
      fix typo. `LCS[j+(blen+1)*j]` -> `LCS[j+(blen+1)*i]`
      719b7665
  2. 20 Apr, 2022 5 commits
    • filipe oliveira's avatar
      Optimization: Use either monotonic or wall-clock to measure command execution... · 3cd8baf6
      filipe oliveira authored
      
      Optimization: Use either monotonic or wall-clock to measure command execution time, to regain up to 4% execution time (#10502)
      
      In #7491 (part of redis 6.2), we started using the monotonic timer instead of mstime to measure
      command execution time for stats, apparently this meant sampling the clock 3 times per command
      rather than two (wince we also need the wall-clock time).
      In some cases this causes a significant overhead.
      
      This PR fixes that by avoiding the use of monotonic timer, except for the cases were we know it
      should be extremely fast.
      This PR also adds a new INFO field called `monotonic_clock` that shows which clock redis is using.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      3cd8baf6
    • Oran Agra's avatar
      Fixes around clients that must be obeyed. Replica report disk errors in PING. (#10603) · ee220599
      Oran Agra authored
      This PR unifies all the places that test if the current client is the
      master client or AOF client, and uses a method to test that on all of
      these.
      
      Other than some refactoring, these are the actual implications:
      - Replicas **don't** ignore disk error when processing commands not
        coming from their master.
        **This is important for PING to be used for health check of replicas**
      - SETRANGE, APPEND, SETBIT, BITFIELD don't do proto_max_bulk_len check for AOF
      - RM_Call in SCRIPT_MODE ignores disk error when coming from master /
        AOF
      - RM_Call in cluster mode ignores slot check when processing AOF
      - Scripts ignore disk error when processing AOF
      - Scripts **don't** ignore disk error on a replica, if the command comes
        from clients other than the master
      - SCRIPT KILL won't kill script coming from AOF
      - Scripts **don't** skip OOM check on replica if the command comes from
        clients other than the master
      
      Note that Script, AOF, and module clients don't reach processCommand,
      which is why some of the changes don't actually have any implications.
      
      Note, reverting the change done to processCommand in 2f4240b9
      should be dead code due to the above mentioned fact.
      ee220599
    • menwen's avatar
      Stop RDB child before flushing and parsing the RDB in Diskless replication too (#10602) · 046654a7
      menwen authored
      
      
      We should stop RDB child in advance before flushing to reduce COW in diskless replication too.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      046654a7
    • yoav-steinberg's avatar
      Optimized `hdr_value_at_percentile` (#10606) · 5075e743
      yoav-steinberg authored
      `hdr_value_at_percentile()` is part of the Hdr_Histogram library
      used when generating `latencystats` report. 
      
      There's a pending optimization for this function which greatly
      affects the performance of `info latencystats`.
      https://github.com/HdrHistogram/HdrHistogram_c/pull/107
      
      
      
      This PR:
      1. Upgrades the sources in _deps/hdr_histogram_ to the latest Hdr_Histogram
        version 0.11.5
      2. Applies the referenced optimization.
      3. Adds minor documentation about the hdr_histogram dependency which was
        missing under _deps/README.md_.
      
      benchmark on my machine:
      running: `redis-benchmark -n 100000 info latencystats` on a clean build with no data.
      
      | benchmark | RPS |
      | ---- | ---- |
      | before upgrade to v0.11.05  | 7,681 |
      | before optimization | 12,474 |
      | after optimization | 52,606 |
      Co-authored-by: default avatarfilipe oliveira <filipecosta.90@gmail.com>
      5075e743
    • David CARLIER's avatar
      Add socket-mark-id support for marking sockets. (#10349) · aba2865c
      David CARLIER authored
      Add a configuration option to attach an operating system-specific identifier to Redis sockets, supporting advanced network configurations using iptables (Linux) or ipfw (FreeBSD).
      aba2865c
  3. 19 Apr, 2022 5 commits
  4. 18 Apr, 2022 2 commits
    • Oran Agra's avatar
      Fix RM_Yield bug processing future commands of the current client. (#10573) · 7d1ad6ca
      Oran Agra authored
      RM_Yield was missing a call to protectClient to prevent redis from
      processing future commands of the yielding client.
      
      Adding tests that fail without this fix.
      
      This would be complicated to solve since nested calls to RM_Call used to
      replace the current_client variable with the module temp client.
      
      It looks like it's no longer necessary to do that, since it was added
      back in #9890 to solve two issues, both already gone:
      1. call to CONFIG SET maxmemory could trigger a module hook calling
         RM_Call. although this specific issue is gone, arguably other hooks
         like keyspace notification, can do the same.
      2. an assertion in lookupKey that checks the current command of the
         current client, introduced in #9572 and removed in #10248
      7d1ad6ca
    • Binbin's avatar
      Fix long long to double implicit conversion warning (#10595) · fe4b4806
      Binbin authored
      
      
      There is a implicit conversion warning in clang:
      ```
      util.c:574:23: error: implicit conversion from 'long long' to 'double'
      changes value from -4611686018427387903 to -4611686018427387904
      [-Werror,-Wimplicit-const-int-float-conversion]
          if (d < -LLONG_MAX/2 || d > LLONG_MAX/2)
      ```
      
      introduced in #10486
      Co-authored-by: default avatarsundb <sundbcn@gmail.com>
      fe4b4806
  5. 17 Apr, 2022 5 commits
    • Oran Agra's avatar
      Optimize integer zset scores in listpack (converting to string and back) (#10486) · 0c4733c8
      Oran Agra authored
      When the score doesn't have fractional part, and can be stored as an integer,
      we use the integer capabilities of listpack to store it, rather than convert it to string.
      This already existed before this PR (lpInsert dose that conversion implicitly).
      
      But to do that, we would have first converted the score from double to string (calling `d2string`),
      then pass the string to `lpAppend` which identified it as being an integer and convert it back to an int.
      Now, instead of converting it to a string, we store it using lpAppendInteger`.
      
      Unrelated:
      ---
      * Fix the double2ll range check (negative and positive ranges, and also the comparison operands
        were slightly off. but also, the range could be made much larger, see comment).
      * Unify the double to string conversion code in rdb.c with the one in util.c
      * Small optimization in lpStringToInt64, don't attempt to convert strings that are obviously too long.
      
      Benchmark;
      ---
      Up to 20% improvement in certain tight loops doing zzlInsert with large integers.
      (if listpack is pre-allocated to avoid realloc, and insertion is sorted from largest to smaller)
      0c4733c8
    • guybe7's avatar
      Add RM_PublishMessageShard (#10543) · f49ff156
      guybe7 authored
      since PUBLISH and SPUBLISH use different dictionaries for channels and clients,
      and we already have an API for PUBLISH, it only makes sense to have one for SPUBLISH
      
      Add test coverage and unifying some test infrastructure.
      f49ff156
    • Meir Shpilraien (Spielrein)'s avatar
      Added test to verify loading Lua binary payload is not possible (#10583) · 789c94fe
      Meir Shpilraien (Spielrein) authored
      The tests verify that loading a binary payload to the Lua interpreter raises an error.
      The Lua code modification was done here: fdf9d455
      which force the Lau interpreter to always use the text parser.
      789c94fe
    • Wang Yuan's avatar
      Optimize the call of prepareReplicasToWrite (#10588) · a9d5cfa9
      Wang Yuan authored
      From #9166, we call several times of prepareReplicasToWrite when propagating
      one write command to replication stream (once per argument, same as we do for
      normal clients), that is not necessary. Now we only call it one time per command
      at the begin of feeding replication stream.
      
      This results in reducing CPU consumption and slightly better performance,
      specifically when there are many replicas.
      a9d5cfa9
    • guybe7's avatar
      Add RM_MallocSizeString, RM_MallocSizeDict (#10542) · fe1c096b
      guybe7 authored
      Add APIs to allow modules to compute the memory consumption of opaque objects owned by redis.
      Without these, the mem_usage callbacks of module data types are useless in many cases.
      
      Other changes:
      Fix streamRadixTreeMemoryUsage to include the size of the rax structure itself
      fe1c096b
  6. 14 Apr, 2022 2 commits
    • Madelyn Olson's avatar
      Fix incorrect error code for eval scripts and fix test error checking (#10575) · effa707e
      Madelyn Olson authored
      By the convention of errors, there is supposed to be a space between the code and the name.
      While looking at some lua stuff I noticed that interpreter errors were not adding the space,
      so some clients will try to map the detailed error message into the error.
      
      We have tests that hit this condition, but they were just checking that the string "starts" with ERR.
      I updated some other tests with similar incorrect string checking. This isn't complete though, as
      there are other ways we check for ERR I didn't fix.
      
      Produces some fun output like:
      ```
      # Errorstats
      errorstat_ERR:count=1
      errorstat_ERRuser_script_1_:count=1
      ```
      effa707e
    • Oran Agra's avatar
      solve corrupt dump fuzzer crash in streams (#10579) · 95050f26
      Oran Agra authored
      we had a panic in streamLastValidID when the stream metadata
      said it's not empty, but the rax is empty.
      95050f26
  7. 13 Apr, 2022 4 commits
    • Wen Hui's avatar
      Fix several document error and function comments (#10580) · ca913a5d
      Wen Hui authored
      
      
      This PR fix the following minor errors before Redis 7 release:
      
      ZRANGEBYLEX command in deprecated in 6.2.0, and could be replaced by ZRANGE with the
      BYLEX argument, but in the document, the words is written incorrect in " by ZRANGE with the BYSCORE argument"
      
      Fix function zpopmaxCommand incorrect comment
      
      The comments of function zmpopCommand and bzmpopCommand are not consistent with document description, fix them
      Co-authored-by: default avatarUbuntu <lucas.guang.yang1@huawei.com>
      ca913a5d
    • filipe oliveira's avatar
      Optimize stream id sds creation on XADD key * (~20% saved cpu cycles) (#10574) · a642947e
      filipe oliveira authored
      
      
      we can observe that when adding to a stream without ID there is a duplicate work
      on sds creation/freeing/sdslen that costs ~11% of the CPU cycles. 
      
      This PR avoids it by not freeing the sds after the first reply. 
      The expected reduction in CPU cycles is around 9-10% 
      
      Additionally, we now pre-allocate the sds to the right size, to avoid realloc.
      this brought another ~10% improvement
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      a642947e
    • Luke Palmer's avatar
      Keyspace event for new keys (#10512) · bb7891f0
      Luke Palmer authored
      Add an optional keyspace event when new keys are added to the db.
      
      This is useful for applications where clients need to be aware of the redis keyspace.
      Such an application can SCAN once at startup and then listen for "new" events (plus
      others associated with DEL, RENAME, etc).
      bb7891f0
    • guybe7's avatar
      Add the deprecated_since field in command args of COMMAND DOCS (#10545) · e875ff89
      guybe7 authored
      Apparently, some modules can afford deprecating command arguments
      (something that was never done in Redis, AFAIK), so in order to represent
      this piece of information, we added the `deprecated_since` field to redisCommandArg
      (in symmetry to the already existing `since` field).
      
      This commit adds `const char *deprecated_since` to `RedisModuleCommandArg`,
      which is technically a breaking change, but since 7.0 was not released yet, we decided to let it slide
      e875ff89
  8. 12 Apr, 2022 3 commits
  9. 11 Apr, 2022 3 commits
    • Ernesto Rodriguez Reina's avatar
      Extending the use of hashTypeGetValue. (#10567) · 4c6d9bbd
      Ernesto Rodriguez Reina authored
      
      
      * Extending the use of hashTypeGetValue.
      
      Functions hashTypeExists, hashTypeGetValueLength and addHashFieldToReply
      have a similar pattern on calling hashTypeGetFromHashTable or
      hashTypeGetFromZipList depending on the underlying data structure. What
      does functions are duing is exactly what hashTypeGetValue does. Those
      functions were changed to use existing function hashTypeGetValue making
      the code more consistent.
      Co-authored-by: default avatarMadelyn Olson <madelyneolson@gmail.com>
      4c6d9bbd
    • zhaozhao.zz's avatar
      Durability enhancement for appendfsync=always policy (#9678) · 1a7765cb
      zhaozhao.zz authored
      Durability of database is a big and old topic, in this regard Redis use AOF to
      support it, and `appendfsync=alwasys` policy is the most strict level, guarantee
      all data is both written and synced on disk before reply success to client.
      
      But there are some cases have been overlooked, and could lead to durability broken.
      
      1. The most clear one is about threaded-io mode
         we should also set client's write handler with `ae_barrier` in
         `handleClientsWithPendingWritesUsingThreads`, or the write handler would be
         called after read handler in the next event loop, it means the write command result
         could be replied to client before flush to AOF.
      2. About blocked client (mostly by module)
         in `beforeSleep()`, `handleClientsBlockedOnKeys()` should be called before
         `flushAppendOnlyFile()`, in case the unblocked clients modify data without persistence
         but send reply.
      3. When handling `ProcessingEventsWhileBlocked`
         normally it takes place when lua/function/module timeout, and we give a chance to users
         to kill the slow operation, but we should call `flushAppendOnlyFile()` before
         `handleClientsWithPendingWrites()`, in case the other clients in the last event loop get
         acknowledge before data persistence.
         for a instance:
         ```
         in the same event loop
         client A executes set foo bar
         client B executes eval "for var=1,10000000,1 do end" 0
         ```
         after the script timeout, client A will get `OK` but lose data after restart (kill redis when
         timeout) if we don't flush the write command to AOF.
      4. A more complex case about `ProcessingEventsWhileBlocked`
         it is lua timeout in transaction, for example
         `MULTI; set foo bar; eval "for var=1,10000000,1 do end" 0; EXEC`, then client will get set
         command's result before the whole transaction done, that breaks atomicity too.
         fortunately, it's already fixed by #5428 (although it's not the original purpose just a side
         effect : )), but module timeout should be fixed too.
      
      case 1, 2, 3 are fixed in this commit, the module issue in case 4 needs a followup PR.
      1a7765cb
    • Evan's avatar
      modules: add RedisModuleKey* return type to RM_OpenKey (#3719) · 574ed6b0
      Evan authored
      Change `RM_OpenKey` to return `RedisModuleKey*` instead of `void*`.
      Which is the input type of other APIs that take the value from RM_OpenKey.
      574ed6b0
  10. 10 Apr, 2022 5 commits
  11. 07 Apr, 2022 2 commits
    • chenyang8094's avatar
      Fix auto-aof-rewrite-percentage based AOFRW trigger after restart (#10550) · 625bdaf3
      chenyang8094 authored
      
      
      The `auto-aof-rewrite-percentage` config defines at what growth percentage
      an automatic AOF rewrite is triggered.
      This normally works OK since the size of the AOF file at the end of a rewrite
      is stored in `server.aof_rewrite_base_size`.
      However, on startup, redis used to store the entire size of the AOF file into that
      variable, resulting in a wrong automatic AOF rewrite trigger (could have been
      triggered much later than desired).
      This issue would only affect the first AOFRW after startup, after that future AOFRW
      would have been triggered correctly.
      This bug existed in all previous versions of Redis.
      
      This PR unifies the meaning of `server.aof_rewrite_base_size`, which only represents
      the size of BASE AOF.
      Note that after an AOFRW this size includes the size of the incremental file (all the
      commands that executed during rewrite), so that auto-aof-rewrite-percentage is the
      ratio from the size of the AOF after rewrite.
      However, on startup, it is complicated to know that size, and we compromised on
      taking just the size of the base file, this means that the first rewrite after startup can
      happen a little bit too soon.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      Co-authored-by: default avataryoav-steinberg <yoav@redislabs.com>
      625bdaf3
    • Oran Agra's avatar
      Fix RM_Yield bug (#10548) · 451531f1
      Oran Agra authored
      The bug was when using REDISMODULE_YIELD_FLAG_CLIENTS.
      in that case we would have only set the CLIENTS type flag in
      server.busy_module_yield_flags and then clear that flag when exiting
      RM_Yield, so we would never call unblockPostponedClients when the
      context is destroyed.
      
      This didn't really have any actual implication, which is why the tests
      couldn't (and still can't) find that since the bug only happens when
      using CLIENT, but in this case we won't have any clients to un-postpone
      i.e. clients will get rejected with BUSY error, rather than being
      postponed.
      
      Unrelated:
      * Adding tests for nested contexts, just in case.
      * Avoid nested RM_Yield calls
      451531f1