1. 18 Dec, 2022 2 commits
    • Oran Agra's avatar
      fix race in list test with blocking commands (#11627) · 669688a3
      Oran Agra authored
      I've seen the `BRPOPLPUSH with multiple blocked clients` test hang.
      this probably happened because rd2 blocked before rd1 and then it was
      also released first, and rd1 remained blocked.
      
      ```
              r del blist{t} target1{t} target2{t}
              r set target1{t} nolist
              $rd1 brpoplpush blist{t} target1{t} 0
              $rd2 brpoplpush blist{t} target2{t} 0
              r lpush blist{t} foo
      
              assert_error "WRONGTYPE*" {$rd1 read}
              assert_equal {foo} [$rd2 read]
              assert_equal {foo} [r lrange target2{t} 0 -1]
      ```
      changes:
      * added all missing calls for wait_for_blocked_client after issuing blocking commands)
      * removed some excessive `after 100`
      * fix undetected crossslot error in BRPOPLPUSH test
      * rollback changes to proto-max-bulk-len so external tests can be rerun
      669688a3
    • Oran Agra's avatar
      fix flaky latency test (#11636) · 60f7111b
      Oran Agra authored
      Fix a flaky test that probably fails on overload timing issues.
      
      This unit starts with
      ```
          # Set a threshold high enough to avoid spurious latency events.
          r config set latency-monitor-threshold 200
      ```
      
      but later the test measuring expire event changes the threshold.
      this fix is to revert it to 200 after that test.
      
      Got this error (ARM+TLS)
      ```
      *** [err]: LATENCY RESET is able to reset events in tests/unit/latency-monitor.tcl
      Expected [r latency latest] eq {} (context: type eval line 3 cmd {assert {[r latency latest] eq {}}} proc ::test)
      ```
      60f7111b
  2. 16 Dec, 2022 1 commit
  3. 15 Dec, 2022 2 commits
  4. 13 Dec, 2022 1 commit
    • Binbin's avatar
      Fix races in swapdb async_loading test (#11613) · 5f69ce0d
      Binbin authored
      
      
      There is a race in the test:
      ```
      *** [err]: Diskless load swapdb (async_loading): new database is exposed after swapping in tests/integration/replication.tcl
      Expected 'myvalue' to be equal to '' (context: type eval line 3 cmd {assert_equal [$replica GET mykey] ""} proc ::test)
      ```
      
      When doing `$replica GET mykey`, the replica is using the old database.
      The reason may be that when doing `master client kill type replica`,
      the replica did not yet realize it got disconnected from the master.
      So the check of master_link_status fails, and the replica did not
      finish the swapdb and the loading.
      
      In that case, i think the solution is to check the sync_full stat on
      the master and wait for it to get incremented from the previous value.
      i.e. the way to know that we're done with the full sync is not to check
      that our state is up (could be up if we check too early), but rather
      check that the sync_full counter got incremented.
      
      During the reviewing, we found another race, in Aborted testType,
      the `$master config set rdb-key-save-delay 10000` is done after we
      already initiated the disconnection, so there's a chance that the replica
      will attempt to reconnect before that call, in which case if we fork() before
      it, the config will not take effect. Move it to above the disconnection.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      5f69ce0d
  5. 12 Dec, 2022 2 commits
    • Oran Agra's avatar
      solve race in replication test due to ping (#11609) · cd12cc2f
      Oran Agra authored
      attach_to_replication_stream already stops pings, but it stops them on
      the server we connect to, and in this case it's a replica, and we need
      to stop them on the real master.
      cd12cc2f
    • Binbin's avatar
      Fix timing issue in replication test (#11611) · ef282bd7
      Binbin authored
      There is a timing issue in the test, happens with valgrind:
      ```
      *** [err]: diskless fast replicas drop during rdb pipe in tests/integration/replication.tcl
      log message of '"*Loading DB in memory*"' not found in ./tests/tmp/server.3580.246/stdout after line: 0 till line: 39
      ```
      
      The server logs:
      ```
      43465:S 03 Dec 2022 01:26:25.664 * Trying a partial resynchronization (request 15155fa24af0539b70428f9b41f4f7129d774560:1).
      43465:S 03 Dec 2022 01:26:35.133 * Full resync from master: 8ddf5a3f7c8ca1061c6b29aa84e7c985c5b29c61:680
      ```
      
      From the logs, we can see it took almost 10s to get full resync response,
      happens with valgrind. it's extremely slow. So i guess it's just an
      insufficient wait_for_condition timeout.
      
      Set the time to 15s, and modify other similar places at the same time.
      ef282bd7
  6. 11 Dec, 2022 1 commit
  7. 09 Dec, 2022 3 commits
    • Binbin's avatar
      Fix zuiFind crash / RM_ScanKey hang on SET object listpack encoding (#11581) · 20854cb6
      Binbin authored
      
      
      In #11290, we added listpack encoding for SET object.
      But forgot to support it in zuiFind, causes ZINTER, ZINTERSTORE,
      ZINTERCARD, ZIDFF, ZDIFFSTORE to crash.
      And forgot to support it in RM_ScanKey, causes it hang.
      
      This PR add support SET listpack in zuiFind, and in RM_ScanKey.
      And add tests for related commands to cover this case.
      
      Other changes:
      - There is no reason for zuiFind to go into the internals of the SET.
        It can simply use setTypeIsMember and don't care about encoding.
      - Remove the `#include "intset.h"` from server.h reduce the chance of
        accidental intset API use.
      - Move setTypeAddAux, setTypeRemoveAux and setTypeIsMemberAux
        interfaces to the header.
      - In scanGenericCommand, use setTypeInitIterator and setTypeNext
        to handle OBJ_SET scan.
      - In RM_ScanKey, improve hash scan mode, use lpGetValue like zset,
        they can share code and better performance.
      
      The zuiFind part fixes #11578
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      Co-authored-by: default avatarViktor Söderqvist <viktor.soderqvist@est.tech>
      20854cb6
    • Oran Agra's avatar
      Solve issues with active defrag test failing on fast machines (#11598) · 528bb11d
      Oran Agra authored
      We do defrag during AOF loading, but aim to detect fragmentation only
      once a second, so this test aims to slow down the AOF loading and mimic
      loading of a large file.
      On fast machines the sleep, plus the actual work we did was insufficient
      making it sleep longer so the test won't fail.
      
      The error we used to get is this one:
      Expected 0 > 100000 (context: type eval line 106 cmd {assert {$hits > 100000}} proc ::test)
      528bb11d
    • filipe oliveira's avatar
      Reduce rewriteClientCommandVector usage on EXPIRE command (#11602) · c3fb48da
      filipe oliveira authored
      
      
      There is overhead on Redis 7.0 EXPIRE command that is not present on 6.2.7. 
      
      We could see that on the unstable profile there are around 7% of CPU cycles
      spent on rewriteClientCommandVector that are not present on 6.2.7.
      This was introduced in #8474.
      This PR reduces the overhead by using 2X rewriteClientCommandArgument instead of
      rewriteClientCommandVector. In this scenario rewriteClientCommandVector creates 4 arguments.
      the above usage of rewriteClientCommandArgument reduces the overhead in half.
      
      This PR should also improve PEXPIREAT performance by avoiding at all
      rewriteClientCommandArgument usage. 
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      c3fb48da
  8. 08 Dec, 2022 3 commits
  9. 07 Dec, 2022 2 commits
    • CatboxParadox's avatar
      Use SNI on outgoing TLS connections (#11458) · 049f5d87
      CatboxParadox authored
      When establishing an outgoing TLS connection using a hostname as a target, use TLS SNI extensions to include the hostname in use.
      049f5d87
    • Harkrishn Patro's avatar
      Optimize client memory usage tracking operation while client eviction is disabled (#11348) · c0267b3f
      Harkrishn Patro authored
      
      
      ## Issue
      During the client input/output buffer processing, the memory usage is
      incrementally updated to keep track of clients going beyond a certain
      threshold `maxmemory-clients` to be evicted. However, this additional
      tracking activity leads to unnecessary CPU cycles wasted when no
      client-eviction is required. It is applicable in two cases.
      
      * `maxmemory-clients` is set to `0` which equates to no client eviction
        (applicable to all clients)
      * `CLIENT NO-EVICT` flag is set to `ON` which equates to a particular
        client not applicable for eviction.  
      
      ## Solution
      * Disable client memory usage tracking during the read/write flow when
        `maxmemory-clients` is set to `0` or `client no-evict` is `on`.
        The memory usage is tracked only during the `clientCron` i.e. it gets
        periodically updated.
      * Cleanup the clients from the memory usage bucket when client eviction
        is disabled.
      * When the maxmemory-clients config is enabled or disabled at runtime,
        we immediately update the memory usage buckets for all clients (tested
        scanning 80000 took some 20ms)
      
      Benchmark shown that this can improve performance by about 5% in
      certain situations.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      c0267b3f
  10. 06 Dec, 2022 2 commits
    • Viktor Söderqvist's avatar
      When converting a set to dict, presize for one more element to be added (#11559) · 8a315fc2
      Viktor Söderqvist authored
      
      
      In most cases when a listpack or intset is converted to a dict, the conversion
      is trigged when adding an element. The extra element is added after conversion
      to dict (in all cases except when the conversion is triggered by
      set-max-intset-entries being reached).
      
      If set-max-listpack-entries is set to a power of two, let's say 128, when
      adding the 129th element, the 128 element listpack is first converted to a dict
      with a hashtable presized for 128 elements. After converting to dict, the 129th
      element is added to the dict which immediately triggers incremental rehashing
      to size 256.
      
      This commit instead presizes the dict to one more element, with the assumption
      that conversion to dict is followed by adding another element, so the dict
      doesn't immediately need rehashing.
      Co-authored-by: default avatarsundb <sundbcn@gmail.com>
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      8a315fc2
    • Binbin's avatar
      Fix command line startup --sentinel problem (#11591) · 8f13ac10
      Binbin authored
      There is a issue with --sentinel:
      ```
      [root]# src/redis-server sentinel.conf --sentinel --loglevel verbose
      
      *** FATAL CONFIG FILE ERROR (Redis 255.255.255) ***
      Reading the configuration file, at line 352
      >>> 'sentinel "--loglevel" "verbose"'
      Unrecognized sentinel configuration statement
      ```
      
      This is because in #10660 (Redis 7.0.1), `--` prefix change break it.
      In this PR, we will handle `--sentinel` the same as we did for `--save`
      in #10866. i.e. it's a pseudo config option with no value.
      8f13ac10
  11. 05 Dec, 2022 2 commits
    • filipe oliveira's avatar
      GEOSEARCH BYBOX: Simplified haversine distance formula when longitude diff is 0 (#11579) · e48ac075
      filipe oliveira authored
      This is take 2 of `GEOSEARCH BYBOX` optimizations based on haversine
      distance formula when longitude diff is 0.
      The first one was in #11535 . 
      
      - Given longitude diff is 0 the asin(sqrt(a)) on the haversine is asin(sin(abs(u))).
      - arcsin(sin(x)) equal to x when x ∈[−𝜋/2,𝜋/2]. 
      - Given latitude is between [−𝜋/2,𝜋/2] we can simplifiy arcsin(sin(x)) to x.
      
      On the sample dataset with 60M datapoints, we've measured 55% increase
      in the achievable ops/sec.
      e48ac075
    • filipe oliveira's avatar
      Reintroduce lua argument cache in luaRedisGenericCommand removed in v7.0 (#11541) · 2d80cd78
      filipe oliveira authored
      This mechanism aims to reduce calls to malloc and free when
      preparing the arguments the script sends to redis commands.
      This is a mechanism was originally implemented in 48c49c48
      and 4f686555
      
      , and was removed in #10220 (thinking it's not needed
      and that it has no impact), but it now turns out it was wrong, and it
      indeed provides some 5% performance improvement.
      
      The implementation is a little bit too simplistic, it assumes consecutive
      calls use the same size in the same arg index, but that's arguably
      sufficient since it's only aimed at caching very small things.
      
      We could even consider always pre-allocating args to the full
      LUA_CMD_OBJCACHE_MAX_LEN (64 bytes) rather than the right size for the argument,
      that would increase the chance they'll be able to be re-used.
      But in some way this is already happening since we're using
      sdsalloc, which in turn uses s_malloc_usable and takes ownership
      of the full side of the allocation, so we are padded to the allocator
      bucket size.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      Co-authored-by: default avatarsundb <sundbcn@gmail.com>
      2d80cd78
  12. 04 Dec, 2022 1 commit
  13. 01 Dec, 2022 2 commits
    • Yossi Gottlieb's avatar
      Improve TLS error handling. (#11563) · 155acef5
      Yossi Gottlieb authored
      * Remove duplicate code, propagating SSL errors into connection state.
      * Add missing error handling in synchronous IO functions.
      * Fix connection error reporting in some replication flows.
      155acef5
    • Binbin's avatar
      Regenerate payloads for cgroups tests using string2printable (#11560) · 79fe450e
      Binbin authored
      The test failed with ERR DUMP payload version or checksum are wrong.
      And it only fails on CentOS, this is due to the fact that tcl8.5 does not correctly
      parse the hexadecimal abbreviation. And in Ubuntu we are using tcl8.6.
      79fe450e
  14. 30 Nov, 2022 4 commits
    • filipe oliveira's avatar
      changing addReplySds and sdscat to addReplyStatusLength() within luaReplyToRedisReply() (#11556) · 68e87eb0
      filipe oliveira authored
      profiling EVALSHA\ we see that luaReplyToRedisReply takes 8.73% out of the
      56.90% of luaCallFunction CPU cycles. 
      
      Using addReplyStatusLength instead of directly composing the protocol to avoid
      sdscatprintf and addReplySds ( which imply multiple sdslen calls ).
      
      The new approach drops
      luaReplyToRedisReply CPU cycles to 3.77%                                                                                                                                           
      68e87eb0
    • Oran Agra's avatar
      Try to fix a race in psync2 test (#11553) · b0250b45
      Oran Agra authored
      This test sets the master ping interval to 1 hour, in order to avoid
      pings in the replicatoin stream incrementing the replication offset,
      however, it didn't increase the repl-timeout so on slow machines
      where the test took more than 60 seconds, the replicas would drop
      and reconnect.
      
      ```
      *** [err]: PSYNC2: Partial resync after restart using RDB aux fields in tests/integration/psync2.tcl
      Replica didn't partial sync
      ```
      
      The test would detect 4 additional partial syncs where it expects
      only one.
      b0250b45
    • guybe7's avatar
      Stream consumers: Re-purpose seen-time, add active-time (#11099) · 72e90695
      guybe7 authored
      1. "Fixed" the current code so that seen-time/idle actually refers to interaction
        attempts (as documented; breaking change)
      2. Added active-time/inactive to refer to successful interaction (what
        seen-time/idle used to be)
      
      At first, I tried to avoid changing the behavior of seen-time/idle but then realized
      that, in this case, the odds are the people read the docs and implemented their
      code based on the docs (which didn't match the behavior).
      For the most part, that would work fine, except that issue #9996 was found.
      
      I was working under the assumption that people relied on the docs, and for
      the most part, it could have worked well enough. so instead of fixing the docs,
      as I would usually do, I fixed the code to match the docs in this particular case.
      
      Note that, in case the consumer has never read any entries, the values
      for both "active-time" (XINFO FULL) and "inactive" (XINFO CONSUMERS) will
      be -1, meaning here that the consumer was never active.
      
      Note that seen/active time is only affected by XREADGROUP / X[AUTO]CLAIM, not
      by XPENDING, XINFO, and other "read-only" stream CG commands (always has been,
      even before this PR)
      
      Other changes:
      * Another behavioral change (arguably a bugfix) is that XREADGROUP and X[AUTO]CLAIM
        create the consumer regardless of whether it was able to perform some reading/claiming
      * RDB format change to save the `active_time`, and set it to the same value of `seen_time` in old rdb files.
      72e90695
    • Huang Zhw's avatar
      Add a special notification unlink available only for modules (#9406) · c8181314
      Huang Zhw authored
      
      
      Add a new module event `RedisModule_Event_Key`, this event is fired
      when a key is removed from the keyspace.
      The event includes an open key that can be used for reading the key before
      it is removed. Modules can also extract the key-name, and use RM_Open
      or RM_Call to access key from within that event, but shouldn't modify anything
      from within this event.
      
      The following sub events are available:
        - `REDISMODULE_SUBEVENT_KEY_DELETED`
        - `REDISMODULE_SUBEVENT_KEY_EXPIRED`
        - `REDISMODULE_SUBEVENT_KEY_EVICTED`
        - `REDISMODULE_SUBEVENT_KEY_OVERWRITE`
      
      The data pointer can be casted to a RedisModuleKeyInfo structure
      with the following fields:
      ```
           RedisModuleKey *key;    // Opened Key
       ```
      
      ### internals
      
      * We also add two dict functions:
        `dictTwoPhaseUnlinkFind` finds an element from the table, also get the plink of the entry.
        The entry is returned if the element is found. The user should later call `dictTwoPhaseUnlinkFree`
        with it in order to unlink and release it. Otherwise if the key is not found, NULL is returned.
        These two functions should be used in pair. `dictTwoPhaseUnlinkFind` pauses rehash and
        `dictTwoPhaseUnlinkFree` resumes rehash.
      * We change `dbOverwrite` to `dbReplaceValue` which just replaces the value of the key and
        doesn't fire any events. The "overwrite" part (which emits events) is just when called from `setKey`,
        the other places that called dbOverwrite were ones that just update the value in-place (INCR*, SPOP,
        and dbUnshareStringValue). This should not have any real impact since `moduleNotifyKeyUnlink` and
        `signalDeletedKeyAsReady` wouldn't have mattered in these cases anyway (i.e. module keys and
        stream keys didn't have direct calls to dbOverwrite)
      * since we allow doing RM_OpenKey from withing these callbacks, we temporarily disable lazy expiry.
      * We also temporarily disable lazy expiry when we are in unlink/unlink2 callback and keyspace 
        notification callback.
      * Move special definitions to the top of redismodule.h
        This is needed to resolve compilation errors with RedisModuleKeyInfoV1
        that carries a RedisModuleKey member.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      c8181314
  15. 29 Nov, 2022 1 commit
    • filipe oliveira's avatar
      Reduce eval related overhead introduced in v7.0 by evalCalcFunctionName (#11521) · 7dfd7b91
      filipe oliveira authored
      
      
      As being discussed in #10981 we see a degradation in performance
      between v6.2 and v7.0 of Redis on the EVAL command. 
      
      After profiling the current unstable branch we can see that we call the
      expensive function evalCalcFunctionName twice. 
      
      The current "fix" is to basically avoid calling evalCalcFunctionName and
      even dictFind(lua_scripts) twice for the same command.
      Instead we cache the current script's dictEntry (for both Eval and Functions)
      in the current client so we don't have to repeat these calls.
      The exception would be when doing an EVAL on a new script that's not yet
      in the script cache. in that case we will call evalCalcFunctionName (and even
      evalExtractShebangFlags) twice.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      7dfd7b91
  16. 28 Nov, 2022 5 commits
    • Mingyi Kang's avatar
      Hyperloglog avoid allocate more than 'server.hll_sparse_max_bytes' bytes of... · f8ac5a65
      Mingyi Kang authored
      Hyperloglog avoid allocate more than 'server.hll_sparse_max_bytes' bytes of memory for sparse representation (#11438)
      
      Before this PR, we use sdsMakeRoomFor() to expand the size of hyperloglog
      string (sparse representation). And because sdsMakeRoomFor() uses a greedy
      strategy (allocate about twice what we need), the memory we allocated for the
      hyperloglog may be more than `server.hll_sparse_max_bytes` bytes.
      The memory more than` server.hll_sparse_max_bytes` will be wasted.
      
      In this pull request, tone down the greediness of the allocation growth, and also
      make sure it'll never request more than `server.hll_sparse_max_bytes`.
      
      This could in theory mean the size of the hyperloglog string is insufficient for the
      increment we need, should be ok since in this case we promote the hyperloglog
      to dense representation, an assertion was added to make sure.
      
      This PR also add some tests and fixes some typo and indentation issues.
      f8ac5a65
    • zhaozhao.zz's avatar
      benchmark getRedisConfig exit only when meet NOAUTH error (#11096) · f0005b53
      zhaozhao.zz authored
      redis-benchmark: when trying to get the CONFIG before benchmark,
      avoid printing any warning on most errors (e.g. NOPERM error).
      avoid aborting the benchmark on NOPERM.
      keep the warning only when we abort the benchmark on a NOAUTH error
      f0005b53
    • Binbin's avatar
      Fix replication on expired key test timing issue, give it more chances (#11548) · 06b577aa
      Binbin authored
      In replica, the key expired before master's `INCR` was arrived, so INCR
      creates a new key in the replica and the test failed.
      ```
      *** [err]: Replication of an expired key does not delete the expired key in tests/integration/replication-4.tcl
      Expected '0' to be equal to '1' (context: type eval line 13 cmd {assert_equal 0 [$slave exists k]} proc ::test)
      ```
      
      This test is very likely to do a false positive if the `wait_for_ofs_sync`
      takes longer than the expiration time, so give it a few more chances.
      
      The test was introduced in #9572.
      06b577aa
    • C Charles's avatar
      Add withscore option to ZRANK and ZREVRANK. (#11235) · eeca7f29
      C Charles authored
      Add an option "withscores" to ZRANK and ZREVRANK.
      
      Add `[withscore]` option to both `zrank` and `zrevrank`, like this:
      ```
      z[rev]rank key member [withscore]
      ```
      eeca7f29
    • filipe oliveira's avatar
      Simplified geoAppendIfWithinShape() and removed spurious calls do sdsdup and sdsfree (#11522) · 376b689b
      filipe oliveira authored
      
      
      In scenarios in which we have large datasets and the elements are not
      contained within the range we do spurious calls do sdsdup and sdsfree.
      I.e. instead of pre-creating an sds before we know if we're gonna use it
      or not, change the role of geoAppendIfWithinShape to just do geoWithinShape,
      and let the caller create the string only when needed.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      376b689b
  17. 27 Nov, 2022 4 commits
  18. 26 Nov, 2022 2 commits