1. 07 Mar, 2023 3 commits
    • Madelyn Olson's avatar
      Always compact nodes in stream listpacks after creating new nodes (#11885) · 2bb29e4a
      Madelyn Olson authored
      This change attempts to alleviate a minor memory usage degradation for Redis 6.2 and onwards when using rather large objects (~2k) in streams. Introduced in #6281, we pre-allocate the head nodes of a stream to be 4kb, to limit the amount of unnecessary initial reallocations that are done. However, if we only ever allocate one object because 2 objects exceeds the max_stream_entry_size, we never actually shrink it to fit the single item. This can lead to a lot of excessive memory usage. For smaller item sizes this becomes less of an issue, as the overhead decreases as the items become smaller in size.
      
      This commit also changes the MEMORY USAGE of streams, since it was reporting the lpBytes instead of the allocated size. This introduced an observability issue when diagnosing the memory issue, since Redis reported the same amount of used bytes pre and post change, even though the new implementation allocated more memory.
      2bb29e4a
    • Binbin's avatar
      Solve race in CLIENT NO-TOUCH lru test (#11883) · 9958ab8b
      Binbin authored
      I've seen it fail here (test-centos7-tls-module-no-tls and test-freebsd):
      ```
      *** [err]: Operations in no-touch mode do not alter the last access time of a key in tests/unit/introspection-2.tcl
      Expected '244296' to be more than '244296' (context: type eval line 12 cmd {assert_morethan $newlru $oldlru} proc ::test)
      ```
      
      Our LRU_CLOCK_RESOLUTION value is 1000ms, and default hz is 10, so if the
      test is really fast, or the timing is just right, newlru will be the same
      as oldlru. We fixed this by changing `after 1000` to `after 1100`.
      9958ab8b
    • sundb's avatar
      Skip test for sdsRemoveFreeSpace when mem_allocator is not jemalloc (#11878) · 3fba3ccd
      sundb authored
      Test `trim on SET with big value` (introduced from #11817) fails under mac m1 with libc mem_allocator.
      The reason is that malloc(33000) will allocate 65536 bytes(>42000).
      This test still passes under ubuntu with libc mem_allocator.
      
      ```
      *** [err]: trim on SET with big value in tests/unit/type/string.tcl
      Expected [r memory usage key] < 42000 (context: type source line 471 file /Users/iospack/data/redis_fork/tests/unit/type/string.tcl cmd {assert {[r memory usage key] < 42000}} proc ::test)
      ```
      
      simple test under mac m1 with libc mem_allocator:
      ```c
      void *p = zmalloc(33000);
      printf("malloc size: %zu\n", zmalloc_size(p));
      
      # output
      malloc size: 65536
      ```
      3fba3ccd
  2. 05 Mar, 2023 1 commit
  3. 04 Mar, 2023 1 commit
    • Binbin's avatar
      Increase the threshold of the AOF loading defrag test (#11871) · bfe50a30
      Binbin authored
      This test is very sensitive and fragile. It often fails in Daily,
      in most cases, it failed in test-ubuntu-32bit (the AOF loading one),
      with the range in (31, 40):
      ```
      [err]: Active defrag in tests/unit/memefficiency.tcl
      Expected 38 <= 30 (context: type eval line 113 cmd {assert {$max_latency <= 30}} proc ::test)
      ```
      
      The AOF loading part isn't tightly fixed to the cron hz. It calls
      processEventsWhileBlocked once in every 1024 command calls.
      ```
              /* Serve the clients from time to time */
              if (!(loops++ % 1024)) {
                  off_t progress_delta = ftello(fp) - last_progress_report_size;
                  loadingIncrProgress(progress_delta);
                  last_progress_report_size += progress_delta;
                  processEventsWhileBlocked();
                  processModuleLoadingProgressEvent(1);
              }
      ```
      
      In this case, we can either decrease the 1024 or increase the
      threshold of just the AOF part of that test. Considering the test
      machines are sometimes slow, and all sort of quirks could happen
      (which do not indicate a bug), and we've already set to 30, we suppose
      we can set it a little bit higher, set it to 40. We can have this instead of
      adding another testing config (we can add it when we really need it).
      
      Fixes #11868
      bfe50a30
  4. 03 Mar, 2023 1 commit
  5. 28 Feb, 2023 4 commits
  6. 26 Feb, 2023 1 commit
    • ranshid's avatar
      assert in case resize output buffer will attempt to shrink too much (#11839) · 4972760b
      ranshid authored
      Currently there is no BUG. However during some internal code changes
      I found that it can happen (for example in case new code will not update
      the buf_peak) which can currently lead to memory overrun which is much
      harder to detect and root cause.
      
      Why did I please the assert here? The reason is to be able to have the
      buf_peak value without the risk of it being overriden by the peak_reset
      4972760b
  7. 23 Feb, 2023 3 commits
  8. 21 Feb, 2023 3 commits
    • Binbin's avatar
      Speed up test: client evicted due to client tracking prefixes (#11823) · cd58af4d
      Binbin authored
      We noticed that `client evicted due to client tracking prefixes`
      takes over 200 seconds with valgrind.
      
      We combine three prefixes in each command, this will probably
      save us half the testing time.
      
      Before: normal: 3508ms, valgrind: 289503ms -> 290s
      With three prefixes, normal: 1500ms, valgrind: 135742ms -> 136s
      
      Since we did not actually count the memory usage of all prefixes, see
      getClientMemoryUsage, so we can not use larger prefixes to speed up the
      test here. Also this PR cleaned up some spaces (IDE jobs) and typos.
      cd58af4d
    • Madelyn Olson's avatar
      Prevent Redis from crashing from key tracking invalidations (#11814) · dca5927a
      Madelyn Olson authored
      There is a built in limit to client side tracking keys, which when exceeded will invalidate keys. This occurs in two places, one in the server cron and other before executing a command. If it happens in the second scenario, the invalidations will be queued for later since current client is set. This queue is never drained if a command is not executed (through call) such as a multi-exec command getting queued. This results in a later server assert crashing.
      dca5927a
    • M Sazzadul Hoque's avatar
      Fix HELLO error message command syntax suggestion (#11809) · 4cc2b0dc
      M Sazzadul Hoque authored
      A simple HELLO command to a password protected Redis server replies
      with an error with another command suggestion. This omits protocol version
      from HELLO command arguments which causes another error.
      This PR adds the protocol version in the command suggestion.
      4cc2b0dc
  9. 20 Feb, 2023 1 commit
  10. 19 Feb, 2023 2 commits
  11. 16 Feb, 2023 3 commits
    • Oran Agra's avatar
      skip new page cache reclame unit test when running in valgrind (#11808) · 5b61b0dc
      Oran Agra authored
      the new test is incompatible with valgrind.
      added a new `--valgrind` argument to `redis-server tests` mode,
      which will cause that test to be skipped..
      5b61b0dc
    • Oran Agra's avatar
      Cleanup around script_caller, fix tracking of scripts and ACL logging for RM_Call (#11770) · 233abbbe
      Oran Agra authored
      * Make it clear that current_client is the root client that was called by
        external connection
      * add executing_client which is the client that runs the current command
        (can be a module or a script)
      * Remove script_caller that was used for commands that have CLIENT_SCRIPT
        to get the client that called the script. in most cases, that's the current_client,
        and in others (when being called from a module), it could be an intermediate
        client when we actually want the original one used by the external connection.
      
      bugfixes:
      * RM_Call with C flag should log ACL errors with the requested user rather than
        the one used by the original client, this also solves a crash when RM_Call is used
        with C flag from a detached thread safe context.
      * addACLLogEntry would have logged info about the script_caller, but in case the
        script was issued by a module command we actually want the current_client. the
        exception is when RM_Call is called from a timer event, in which case we don't
        have a current_client.
      
      behavior changes:
      * client side tracking for scripts now tracks the keys that are read by the script
        instead of the keys that are declared by the caller for EVAL
      
      other changes:
      * Log both current_client and executing_client in the crash log.
      * remove prepareLuaClient and resetLuaClient, being dead code that was forgotten.
      * remove scriptTimeSnapshot and snapshot_time and instead add cmd_time_snapshot
        that serves all commands and is reset only when execution nesting starts.
      * remove code to propagate CLIENT_FORCE_REPL from the executed command
        to the script caller since scripts aren't propagated anyway these days and anyway
        this flag wouldn't have had an effect since CLIENT_PREVENT_PROP is added by scriptResetRun.
      * fix a module GIL violation issue in afterSleep that was introduced in #10300 (unreleased)
      233abbbe
    • zhaozhao.zz's avatar
      a35e0837
  12. 15 Feb, 2023 1 commit
    • Binbin's avatar
      Remove wrong code in list pot timeout test (#11805) · 7d5382c0
      Binbin authored
      In #9373, actually need to replace `$rd $pop blist1{t} blist2{t} 1`
      with `bpop_command_two_key $rd $pop blist1{t} blist2{t} 1` but forgot
      to delete the latter.
      
      This doesn't affect the test, because the later assert_error "WRONGTYPE"
      is expected (and right). And if we read $rd again, it will get the
      wrong result, like 'ERR unknown command 'BLMPOP_LEFT' | 'BLMPOP_RIGHT'
      7d5382c0
  13. 14 Feb, 2023 3 commits
    • Wen Hui's avatar
      Update codes (#11804) · a7051845
      Wen Hui authored
      In this PR, we use function pointer *isPresent replace the variable "present" in auxFieldHandler, so that in the future, when we have more aux fields, we could decide if the aux field is displayed or not.
      a7051845
    • guybe7's avatar
      Minor changes around the blockonkeys test module (#11803) · 9483ab0b
      guybe7 authored
      All of the POP commands must not decr length below 0.
      So, get_fsl will delete the key if the length is 0 (unless
      the caller wished to create if doesn't exist)
      
      Other:
      1. Use REDISMODULE_WRITE where needed (POP commands)
      2. Use wait_for_blokced_clients in test
      
      Unrelated:
      Use quotes instead of curly braces in zset.tcl, for variable expansion
      9483ab0b
    • guybe7's avatar
      SCAN/RANDOMKEY and lazy-expire (#11788) · fd82bccd
      guybe7 authored
      Starting from Redis 7.0 (#9890) we started wrapping everything a command
       propagates with MULTI/EXEC. The problem is that both SCAN and RANDOMKEY can
      lazy-expire arbitrary keys (similar behavior to active-expire), and put DELs in a transaction.
      
      Fix: When these commands are called without a parent exec-unit (e.g. not in EVAL or
      MULTI) we avoid wrapping their DELs in a transaction (for the same reasons active-expire
      and eviction avoids a transaction)
      
      This PR adds a per-command flag that indicates that the command may touch arbitrary
      keys (not the ones in the arguments), and uses that flag to avoid the MULTI-EXEC.
      For now, this flag is internal, since we're considering other solutions for the future.
      
      Note for cluster mode: if SCAN/RANDOMKEY is inside EVAL/MULTI it can still cause the
      same situation (as it always did), but it won't cause a CROSSSLOT because replicas and AOF
      do not perform slot checks.
      The problem with the above is mainly for 3rd party ecosystem tools that propagate commands
      from master to master, or feed an AOF file with redis-cli into a master.
      This PR aims to fix the regression in redis 7.0, and we opened #11792 to try to handle the
      bigger problem with lazy expire better for another release.
      fd82bccd
  14. 12 Feb, 2023 1 commit
    • Tian's avatar
      Reclaim page cache of RDB file (#11248) · 7dae142a
      Tian authored
      # Background
      The RDB file is usually generated and used once and seldom used again, but the content would reside in page cache until OS evicts it. A potential problem is that once the free memory exhausts, the OS have to reclaim some memory from page cache or swap anonymous page out, which may result in a jitters to the Redis service.
      
      Supposing an exact scenario, a high-capacity machine hosts many redis instances, and we're upgrading the Redis together. The page cache in host machine increases as RDBs are generated. Once the free memory drop into low watermark(which is more likely to happen in older Linux kernel like 3.10, before [watermark_scale_factor](https://lore.kernel.org/lkml/1455813719-2395-1-git-send-email-hannes@cmpxchg.org/) is introduced, the `low watermark` is linear to `min watermark`, and there'is not too much buffer space for `kswapd` to be wake up to reclaim memory), a `direct reclaim` happens, which means the process would stall to wait for memory allocation.
      
      # What the PR does
      The PR introduces a capability to reclaim the cache when the RDB is operated. Generally there're two cases, read and write the RDB. For read it's a little messy to address the incremental reclaim, so the reclaim is done in one go in background after the load is finished to avoid blocking the work thread. For write, incremental reclaim amortizes the work of reclaim so no need to put it into background, and the peak watermark of cache can be reduced in this way.
      
      Two cases are addresses specially, replication and restart, for both of which the cache is leveraged to speed up the processing, so the reclaim is postponed to a right time. To do this, a flag is added to`rdbSave` and `rdbLoad` to control whether the cache need to be kept, with the default value false.
      
      # Something deserve noting
      1. Though `posix_fadvise` is the POSIX standard, but only few platform support it, e.g. Linux, FreeBSD 10.0.
      2. In Linux `posix_fadvise` only take effect on writeback-ed pages, so a `sync`(or `fsync`, `fdatasync`) is needed to flush the dirty page before `posix_fadvise` if we reclaim write cache.
      
      # About test
      A unit test is added to verify the effect of `posix_fadvise`.
      In integration test overall cache increase is checked, as well as the cache backed by RDB as a specific TCL test is executed in isolated Github action job.
      7dae142a
  15. 09 Feb, 2023 2 commits
    • Meir Shpilraien (Spielrein)'s avatar
      Match REDISMODULE_OPEN_KEY_* flags to LOOKUP_* flags (#11772) · 5c3938d5
      Meir Shpilraien (Spielrein) authored
      The PR adds support for the following flags on RedisModule_OpenKey:
      
      * REDISMODULE_OPEN_KEY_NONOTIFY - Don't trigger keyspace event on key misses.
      * REDISMODULE_OPEN_KEY_NOSTATS - Don't update keyspace hits/misses counters.
      * REDISMODULE_OPEN_KEY_NOEXPIRE - Avoid deleting lazy expired keys.
      * REDISMODULE_OPEN_KEY_NOEFFECTS - Avoid any effects from fetching the key
      
      In addition, added `RM_GetOpenKeyModesAll`, which returns the mask of all
      supported OpenKey modes. This allows the module to check, in runtime, which
      OpenKey modes are supported by the current Redis instance.
      5c3938d5
    • Binbin's avatar
      When DEBUG LOADAOF fails, return an error instead of exiting (#11790) · 66bed3f2
      Binbin authored
      Return an error when loadAppendOnlyFiles fails instead of
      exiting. DEBUF LOADAOF command is only meant to be used by
      the test suite, and only by tests that generated an AOF file
      first. So this change is ok (considering that the caller is
      likely to catch this error and die).
      
      This actually revert part of the code in #9012, and now
      DEBUG LOADAOF behaves the same as DEBUG RELOAD (returns an
      error when the load fails).
      
      Plus remove a `after 2000` in a test, which can save times (looks like copy paste error).
      66bed3f2
  16. 06 Feb, 2023 1 commit
  17. 03 Feb, 2023 2 commits
    • Binbin's avatar
      Fix unstable test: replication with parallel clients writing in different DBs (#11782) · 03347d04
      Binbin authored
      Failure happens in FreeBSD daily:
      ```
      *** [err]: Test replication with parallel clients writing in different DBs in tests/integration/replication-4.tcl
      Expected [::redis::redisHandle2 dbsize] > 0 (context: type eval line 19 cmd {assert {[$master dbsize] > 0}} proc ::test)
      ```
      
      The test is failing because db 9 has no data (default db), and
      according to the log, we can see that db 9 does not have a key:
      ```
       ### Starting test Test replication with parallel clients writing in different DBs in tests/integration/replication-4.tcl
      3338:S 03 Feb 2023 00:15:18.723 - DB 11: 1 keys (0 volatile) in 4 slots HT.
      3338:S 03 Feb 2023 00:15:18.723 - DB 12: 141 keys (0 volatile) in 256 slots HT.
      ```
      
      We use `wait_for_condition` to ensure that parallel clients have
      written data before calling stop_bg_complex_data. At the same time,
      `wait_for_condition` is also used to remove the above `after 1000`,
      which can save time in most cases.
      03347d04
    • Binbin's avatar
      Fix timing issue in new ACL log test (#11781) · 5a3cdddd
      Binbin authored
      There is a timing issue in the new ACL log test:
      ```
      *** [err]: ACL LOG aggregates similar errors together and assigns unique entry-id to new errors in tests/unit/acl.tcl
      Expected 1675382873989 < 1675382873989 (context: type eval line 15 cmd {assert {$timestamp_last_update_original < $timestamp_last_updated_after_update}} proc ::test)
      ```
      
      Looking at the test code, we will check the `timestamp-last-updated` before
      and after a new ACL error occurs. Actually `WRONGPASS` errors can be executed
      very quickly on fast machines. For example, in the this case, the execution is
      completed within one millisecond.
      
      The error is easy to reproduce, if we reduce the number of the for loops, for
      example set to 2, and using --loop and --stop. Avoid this timing issue by adding
      an `after 1` before the new errors.
      
      The test was introduced in #11477.
      5a3cdddd
  18. 02 Feb, 2023 4 commits
    • Roshan Khatri's avatar
      Added fields to ACL LOG error entries for precise time logging (#11477) · ac312954
      Roshan Khatri authored
      
      
      Added 3 fields to the ACL LOG - adds entry_id, timestamp_created and timestamp_last_updated, which updates similar existing log error entries. The pair - entry_id, timestamp_created is a unique identifier of this entry, in case the node dies and is restarted, it can detect that if it's a new series.
      
      The primary use case of Unique id is to uniquely identify the error messages and not to detect if the server has restarted.
      
      entry-id is the sequence number of the entry (starting at 0) since the server process started. Can also be used to check if items were "lost" if they fell between periods.
      timestamp-created is the unix-time in ms at the time the entry was first created.
      timestamp-last-updated is the unix-time in ms at the time the entry was last updated
      Time_created gives the absolute time which better accounts for network time as compared to time since. It can also be older than 60 secs and presently there is no field that can display the original time of creation once the error entry is updated.
      The reason of timestamp_last_updated field is that it provides a more precise value for the “last time” an error was seen where as, presently it is only in the 60 second period.
      Co-authored-by: default avatarMadelyn Olson <madelyneolson@gmail.com>
      ac312954
    • vanguard_space's avatar
      adding the ability to add streams to the pre-defined redis-benchmark tests (#11762) · 3b260149
      vanguard_space authored
      Added standard way to support xadd as one of the commands that can be run via redis-benchmarking tool
      3b260149
    • Harkrishn Patro's avatar
      Propagate message to a node only if the cluster link is healthy. (#11752) · fd397568
      Harkrishn Patro authored
      Currently while a sharded pubsub message publish tries to propagate the message across the cluster, a NULL check is missing for clusterLink. clusterLink could be NULL if the link is causing memory beyond the set threshold cluster-link-sendbuf-limit and server terminates the link.
      
      This change introduces two things:
      
      Avoids the engine crashes on the publishing node if a message is tried to be sent to a node and the link is NULL.
      Adds a debugging tool CLUSTERLINK KILL to terminate the clusterLink between two nodes.
      fd397568
    • Florian Weimer's avatar
      deps/jemalloc: Do not force building in gnu99 mode (#11583) · 023ff42f
      Florian Weimer authored
      
      
      Previously, jemalloc was explicitly configured to build in `gnu99` mode. As a result, `<stdatomic.h>` was presumed to be unavailable and never used.
      
      This commit removes explicit build flags configuration and lets `autoconf` determine the supported build flags. In addition, we also no longer build C++ jemalloc code.
      Co-authored-by: default avatarYossi Gottlieb <yossigo@gmail.com>
      023ff42f
  19. 01 Feb, 2023 2 commits
    • Binbin's avatar
      Fix handshake timeout replication test race (#11773) · ffb691f6
      Binbin authored
      Test on x86 + TLS fail with this error:
      ```
      *** [err]: Slave is able to detect timeout during handshake in tests/integration/replication.tcl
      Replica is not able to detect timeout
      ```
      
      The replica logs is:
      ```
       ### Starting test Slave is able to detect timeout during handshake in tests/integration/replication.tcl
      7681:S 05 Jan 2023 00:21:56.635 * Non blocking connect for SYNC fired the event.
      7681:S 05 Jan 2023 00:21:56.638 * Master replied to PING, replication can continue...
      7681:S 05 Jan 2023 00:21:56.638 * Trying a partial resynchronization (request ef70638885500aad12dd673c68ca1541116a59fe:1).
      7681:S 05 Jan 2023 00:22:56.894 # Failed to read response from the server: error:0A000126:SSL routines::unexpected eof while reading
      7681:S 05 Jan 2023 00:22:56.894 # Master did not reply to PSYNC, will try later
      ```
      
      This is another issue that appeared after #11640 was merged. This PR try to fix it.
      The idea is to make it stable in `wait_bgsave`, for example, it may wait until the
      next psync retry in the following situation: `Master did not reply to PSYNC, will try later`
      
      Other than that, the change will make the test more consistent / predictable since
      it'll mean the master is always frozen in the desired state (waiting for repl-diskless-sync-delay
      to happen, rather than earlier stages of the handshake).
      ffb691f6
    • Binbin's avatar
      Document some fields history of CLIENT LIST command (#11729) · e7f35edb
      Binbin authored
      Change history:
      - `user` added in 6.0.0, 0f42447a
      - `argv-mem` and `tot-mem` added in 6.2.0, bea40e6a
      - `redir` added in 6.2.0, dd1f20ed
      - `resp` added in 7.0.0, 7c376398
      - `multi-mem` added in 7.0.0, 2753429c
      - `rbs` and `rbp` added in 7.0.0, 47c51d0c
      - `ssub` added in 7.0.3, 35c2ee87
      e7f35edb
  20. 31 Jan, 2023 1 commit
    • uriyage's avatar
      Optimization: sdsRemoveFreeSpace to avoid realloc on noop (#11766) · 46393f98
      uriyage authored
      
      
      In #7875 (Redis 6.2), we changed the sds alloc to be the usable allocation
      size in order to:
      
      > reduce the need for realloc calls by making the sds implicitly take over
      the internal fragmentation
      
      This change was done most sds functions, excluding `sdsRemoveFreeSpace` and
      `sdsResize`, the reason is that in some places (e.g. clientsCronResizeQueryBuffer)
      we call sdsRemoveFreeSpace when we see excessive free space and want to trim it.
      so if we don't trim it exactly to size, the caller may still see excessive free space and
      call it again and again.
      
      However, this resulted in some excessive calls to realloc, even when there's no need
      and it's gonna be a no-op (e.g. when reducing 15 bytes allocation to 13).
      
      It turns out that a call for realloc with jemalloc can be expensive even if it ends up
      doing nothing, so this PR adds a check using `je_nallocx`, which is cheap to avoid
      the call for realloc.
      
      in addition to that this PR unifies sdsResize and sdsRemoveFreeSpace into common
      code. the difference between them was that sdsResize would avoid using SDS_TYPE_5,
      since it want to keep the string ready to be resized again, while sdsRemoveFreeSpace
      would permit using SDS_TYPE_5 and get an optimal memory consumption.
      now both methods take a `would_regrow` argument that makes it more explicit.
      
      the only actual impact of that is that in clientsCronResizeQueryBuffer we call both sdsResize
      and sdsRemoveFreeSpace for in different cases, and we now prevent the use of SDS_TYPE_5 in both.
      
      The new test that was added to cover this concern used to pass before this PR as well,
      this PR is just a performance optimization and cleanup.
      
      Benchmark:
      `redis-benchmark -c 100 -t set  -d 512 -P 10  -n  100000000`
      on i7-9850H with jemalloc, shows improvement from 1021k ops/sec to 1067k (average of 3 runs).
      some 4.5% improvement.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      46393f98