1. 02 Jan, 2023 1 commit
  2. 01 Jan, 2023 1 commit
    • ranshid's avatar
      reprocess command when client is unblocked on keys (#11012) · 383d902c
      ranshid authored
      *TL;DR*
      ---------------------------------------
      Following the discussion over the issue [#7551](https://github.com/redis/redis/issues/7551
      
      )
      We decided to refactor the client blocking code to eliminate some of the code duplications
      and to rebuild the infrastructure better for future key blocking cases.
      
      
      *In this PR*
      ---------------------------------------
      1. reprocess the command once a client becomes unblocked on key (instead of running
         custom code for the unblocked path that's different than the one that would have run if
         blocking wasn't needed)
      2. eliminate some (now) irrelevant code for handling unblocking lists/zsets/streams etc...
      3. modify some tests to intercept the error in cases of error on reprocess after unblock (see
         details in the notes section below)
      4. replace '$' on the client argv with current stream id. Since once we reprocess the stream
         XREAD we need to read from the last msg and not wait for new msg  in order to prevent
         endless block loop. 
      5. Added statistics to the info "Clients" section to report the:
         * `total_blocking_keys` - number of blocking keys
         * `total_blocking_keys_on_nokey` - number of blocking keys which have at least 1 client
            which would like
         to be unblocked on when the key is deleted.
      6. Avoid expiring unblocked key during unblock. Previously we used to lookup the unblocked key
         which might have been expired during the lookup. Now we lookup the key using NOTOUCH and
         NOEXPIRE to avoid deleting it at this point, so propagating commands in blocked.c is no longer needed.
      7. deprecated command flags. We decided to remove the CMD_CALL_STATS and CMD_CALL_SLOWLOG
         and make an explicit verification in the call() function in order to decide if stats update should take place.
         This should simplify the logic and also mitigate existing issues: for example module calls which are
         triggered as part of AOF loading might still report stats even though they are called during AOF loading.
      
      *Behavior changes*
      ---------------------------------------------------
      
      1. As this implementation prevents writing dedicated code handling unblocked streams/lists/zsets,
      since we now re-process the command once the client is unblocked some errors will be reported differently.
      The old implementation used to issue
      ``UNBLOCKED the stream key no longer exists``
      in the following cases:
         - The stream key has been deleted (ie. calling DEL)
         - The stream and group existed but the key type was changed by overriding it (ie. with set command)
         - The key not longer exists after we swapdb with a db which does not contains this key
         - After swapdb when the new db has this key but with different type.
         
      In the new implementation the reported errors will be the same as if the command was processed after effect:
      **NOGROUP** - in case key no longer exists, or **WRONGTYPE** in case the key was overridden with a different type.
      
      2. Reprocessing the command means that some checks will be reevaluated once the
      client is unblocked.
      For example, ACL rules might change since the command originally was executed and
      will fail once the client is unblocked.
      Another example is OOM condition checks which might enable the command to run and
      block but fail the command reprocess once the client is unblocked.
      
      3. One of the changes in this PR is that no command stats are being updated once the
      command is blocked (all stats will be updated once the client is unblocked). This implies
      that when we have many clients blocked, users will no longer be able to get that information
      from the command stats. However the information can still be gathered from the client list.
      
      **Client blocking**
      ---------------------------------------------------
      
      the blocking on key will still be triggered the same way as it is done today.
      in order to block the current client on list of keys, the call to
      blockForKeys will still need to be made which will perform the same as it is today:
      
      *  add the client to the list of blocked clients on each key
      *  keep the key with a matching list node (position in the global blocking clients list for that key)
         in the client private blocking key dict.
      *  flag the client with CLIENT_BLOCKED
      *  update blocking statistics
      *  register the client on the timeout table
      
      **Key Unblock**
      ---------------------------------------------------
      
      Unblocking a specific key will be triggered (same as today) by calling signalKeyAsReady.
      the implementation in that part will stay the same as today - adding the key to the global readyList.
      The reason to maintain the readyList (as apposed to iterating over all clients blocked on the specific key)
      is in order to keep the signal operation as short as possible, since it is called during the command processing.
      The main change is that instead of going through a dedicated code path that operates the blocked command
      we will just call processPendingCommandsAndResetClient.
      
      **ClientUnblock (keys)**
      ---------------------------------------------------
      
      1. Unblocking clients on keys will be triggered after command is
         processed and during the beforeSleep
      8. the general schema is:
      9. For each key *k* in the readyList:
      ```            
      For each client *c* which is blocked on *k*:
                  in case either:
      	          1. *k* exists AND the *k* type matches the current client blocking type
      	  	      OR
      	          2. *k* exists and *c* is blocked on module command
      	    	      OR
      	          3. *k* does not exists and *c* was blocked with the flag
      	             unblock_on_deleted_key
                       do:
                                        1. remove the client from the list of clients blocked on this key
                                        2. remove the blocking list node from the client blocking key dict
                                        3. remove the client from the timeout list
                                        10. queue the client on the unblocked_clients list
                                        11. *NEW*: call processCommandAndResetClient(c);
      ```
      *NOTE:* for module blocked clients we will still call the moduleUnblockClientByHandle
                    which will queue the client for processing in moduleUnblockedClients list.
      
      **Process Unblocked clients**
      ---------------------------------------------------
      
      The process of all unblocked clients is done in the beforeSleep and no change is planned
      in that part.
      
      The general schema will be:
      For each client *c* in server.unblocked_clients:
      
              * remove client from the server.unblocked_clients
              * set back the client readHandler
              * continue processing the pending command and input buffer.
      
      *Some notes regarding the new implementation*
      ---------------------------------------------------
      
      1. Although it was proposed, it is currently difficult to remove the
         read handler from the client while it is blocked.
         The reason is that a blocked client should be unblocked when it is
         disconnected, or we might consume data into void.
      
      2. While this PR mainly keep the current blocking logic as-is, there
         might be some future additions to the infrastructure that we would
         like to have:
         - allow non-preemptive blocking of client - sometimes we can think
           that a new kind of blocking can be expected to not be preempt. for
           example lets imagine we hold some keys on disk and when a command
           needs to process them it will block until the keys are uploaded.
           in this case we will want the client to not disconnect or be
           unblocked until the process is completed (remove the client read
           handler, prevent client timeout, disable unblock via debug command etc...).
         - allow generic blocking based on command declared keys - we might
           want to add a hook before command processing to check if any of the
           declared keys require the command to block. this way it would be
           easier to add new kinds of key-based blocking mechanisms.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      Signed-off-by: default avatarRan Shidlansik <ranshid@amazon.com>
      383d902c
  3. 28 Dec, 2022 1 commit
    • sundb's avatar
      Remove unnecessary updateClientMemUsageAndBucket() when feeding monitors (#11657) · af0a4fe2
      sundb authored
      This call is introduced in #8687, but became irrelevant in #11348, and is currently a no-op.
      The fact is that #11348 an unintended side effect, which is that even if the client eviction config
      is enabled, there are certain types of clients for which memory consumption is not accurately
      tracked, and so unlike normal clients, their memory isn't reported correctly in INFO.
      af0a4fe2
  4. 20 Dec, 2022 1 commit
    • guybe7's avatar
      Cleanup: Get rid of server.core_propagates (#11572) · 9c7c6924
      guybe7 authored
      1. Get rid of server.core_propagates - we can just rely on module/call nesting levels
      2. Rename in_nested_call  to execution_nesting and update the comment
      3. Remove module_ctx_nesting (redundant, we can use execution_nesting)
      4. Modify postExecutionUnitOperations according to the comment (The main purpose of this PR)
      5. trackingHandlePendingKeyInvalidations: Check the nesting level inside this function
      9c7c6924
  5. 15 Dec, 2022 2 commits
  6. 09 Dec, 2022 2 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
    • 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
  7. 08 Dec, 2022 3 commits
  8. 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
  9. 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
  10. 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
  11. 04 Dec, 2022 1 commit
  12. 01 Dec, 2022 1 commit
    • 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
  13. 30 Nov, 2022 3 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
    • 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
  14. 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
  15. 28 Nov, 2022 4 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
    • 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
  16. 27 Nov, 2022 3 commits
  17. 26 Nov, 2022 3 commits
  18. 25 Nov, 2022 1 commit
  19. 24 Nov, 2022 5 commits
    • Meir Shpilraien (Spielrein)'s avatar
      Module API to allow writes after key space notification hooks (#11199) · abc345ad
      Meir Shpilraien (Spielrein) authored
      ### Summary of API additions
      
      * `RedisModule_AddPostNotificationJob` - new API to call inside a key space
        notification (and on more locations in the future) and allow to add a post job as describe above.
      * New module option, `REDISMODULE_OPTIONS_ALLOW_NESTED_KEYSPACE_NOTIFICATIONS`,
        allows to disable Redis protection of nested key-space notifications.
      * `RedisModule_GetModuleOptionsAll` - gets the mask of all supported module options so a module
        will be able to check if a given option is supported by the current running Redis instance.
      
      ### Background
      
      The following PR is a proposal of handling write operations inside module key space notifications.
      After a lot of discussions we came to a conclusion that module should not perform any write
      operations on key space notification.
      
      Some examples of issues that such write operation can cause are describe on the following links:
      
      * Bad replication oreder - https://github.com/redis/redis/pull/10969
      * Used after free - https://github.com/redis/redis/pull/10969#issuecomment-1223771006
      * Used after free - https://github.com/redis/redis/pull/9406#issuecomment-1221684054
      
      
      
      There are probably more issues that are yet to be discovered. The underline problem with writing
      inside key space notification is that the notification runs synchronously, this means that the notification
      code will be executed in the middle on Redis logic (commands logic, eviction, expire).
      Redis **do not assume** that the data might change while running the logic and such changes
      can crash Redis or cause unexpected behaviour.
      
      The solution is to state that modules **should not** perform any write command inside key space
      notification (we can chose whether or not we want to force it). To still cover the use-case where
      module wants to perform a write operation as a reaction to key space notifications, we introduce
      a new API , `RedisModule_AddPostNotificationJob`, that allows to register a callback that will be
      called by Redis when the following conditions hold:
      
      * It is safe to perform any write operation.
      * The job will be called atomically along side the operation that triggers it (in our case, key
        space notification).
      
      Module can use this new API to safely perform any write operation and still achieve atomicity
      between the notification and the write.
      
      Although currently the API is supported on key space notifications, the API is written in a generic
      way so that in the future we will be able to use it on other places (server events for example).
      
      ### Technical Details
      
      Whenever a module uses `RedisModule_AddPostNotificationJob` the callback is added to a list
      of callbacks (called `modulePostExecUnitJobs`) that need to be invoke after the current execution
      unit ends (whether its a command, eviction, or active expire). In order to trigger those callback
      atomically with the notification effect, we call those callbacks on `postExecutionUnitOperations`
      (which was `propagatePendingCommands` before this PR). The new function fires the post jobs
      and then calls `propagatePendingCommands`.
      
      If the callback perform more operations that triggers more key space notifications. Those keys
      space notifications might register more callbacks. Those callbacks will be added to the end
      of `modulePostExecUnitJobs` list and will be invoke atomically after the current callback ends.
      This raises a concerns of entering an infinite loops, we consider infinite loops as a logical bug
      that need to be fixed in the module, an attempt to protect against infinite loops by halting the
      execution could result in violation of the feature correctness and so **Redis will make no attempt
      to protect the module from infinite loops**
      
      In addition, currently key space notifications are not nested. Some modules might want to allow
      nesting key-space notifications. To allow that and keep backward compatibility, we introduce a
      new module option called `REDISMODULE_OPTIONS_ALLOW_NESTED_KEYSPACE_NOTIFICATIONS`.
      Setting this option will disable the Redis key-space notifications nesting protection and will
      pass this responsibility to the module.
      
      ### Redis infrastructure
      
      This PR promotes the existing `propagatePendingCommands` to an "Execution Unit" concept,
      which is called after each atomic unit of execution,
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      Co-authored-by: default avatarYossi Gottlieb <yossigo@gmail.com>
      Co-authored-by: default avatarMadelyn Olson <34459052+madolson@users.noreply.github.com>
      abc345ad
    • filipe oliveira's avatar
      GEOSEARCH BYBOX: Reduce wastefull computation on... · ae1de549
      filipe oliveira authored
      
      GEOSEARCH BYBOX: Reduce wastefull computation on geohashGetDistanceIfInRectangle and geohashGetDistance (#11535)
      
      Optimize geohashGetDistanceIfInRectangle when there are many misses.
      It calls 3x geohashGetDistance. The first 2 times we call them to produce intermediate results.
      This PR focus on optimizing for those 2 intermediate results.
      
      1 Reduce expensive computation on intermediate geohashGetDistance with same long
      2 Avoid expensive lon_distance calculation if lat_distance fails beforehand
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      ae1de549
    • Binbin's avatar
      Fix sanitizer warning, use offsetof instread of member_offset (#11539) · ca174e1d
      Binbin authored
      
      
      In #11511 we introduced member_offset which has a sanitizer warning:
      ```
      multi.c:390:26: runtime error: member access within null pointer of type 'watchedKey' (aka 'struct watchedKey')
      SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior multi.c:390:26
      ```
      
      We can use offsetof() from stddef.h. This is part of the standard lib
      just to avoid this UB :) Sanitizer should not complain after we change
      this.
      
      1. Use offsetof instead of member_offset, so we can delete this now
      2. Changed (uint8_t*) cast to (char*).
      
      This does not matter much but according to standard, we are only allowed
      to cast pointers to its own type, char* and void*. Let's try to follow
      the rules.
      
      This change was suggested by tezc and the comments is also from him.
      Co-authored-by: default avatarOzan Tezcan <ozantezcan@gmail.com>
      ca174e1d
    • sundb's avatar
      Ignore -Wstringop-overread warning for SHA1Transform() on GCC 12 (#11538) · fd808185
      sundb authored
      Fix compile warning for SHA1Transform() method under alpine with GCC 12.
      
      Warning:
      ```
      In function 'SHA1Update',
          inlined from 'SHA1Final' at sha1.c:187:9:
      sha1.c:144:13: error: 'SHA1Transform' reading 64 bytes from a region of size 0 [-Werror=stringop-overread]
        144 |             SHA1Transform(context->state, &data[i]);
            |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      sha1.c:144: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])
            |      ^~~~~~~~~~~~~
      ```
      
      This warning is a false positive because it has been determined in the loop judgment that there must be 64 chars after position `i`
      ```c
      for ( ; i + 63 < len; i += 64) {
          SHA1Transform(context->state, &data[i]);
      }
      ```
      
      Reference: https://github.com/libevent/libevent/commit/e1d7d3e40a7fd50348d849046fbfd9bf976e643c
      fd808185
    • Wen Hui's avatar
      Update Sentinel Debug command json file and add test case for it (#11513) · 75c66fb0
      Wen Hui authored
      Command SENTINEL DEBUG could be no arguments, which display all
      configurable arguments and their values.
      Update the command arguments in the docs (json file) to indicate that
      arguments are optional
      75c66fb0
  20. 23 Nov, 2022 1 commit
    • Mingyi Kang's avatar
      optimize unwatchAllKeys() (#11511) · 3b462ce5
      Mingyi Kang authored
      In unwatchAllKeys() function, we traverse all the keys watched by the client,
      and for each key we need to remove the client from the list of clients watching that key.
      This is implemented by listSearchKey which traverses the list of clients.
      
      If we can reach the node of the list of clients from watchedKey in O(1) time,
      then we do not need to call listSearchKey anymore.
      
      Changes in this PR: put the node of the list of clients of each watched key in the
      db inside the watchedKey structure. In this way, for every key watched by the client,
      we can get the watchedKey structure and then reach the node in the list of clients in
      db->watched_keys to remove it from that list.
      From the perspective of the list of clients watching the key, the list node is inside a
      watchedKey structure, so we can get to the watchedKey struct from the listnode by
      struct member offset math. And because of this, node->value is not used, we can point
      node->value to the list itself, so that we don't need to fetch the list of clients from the dict.
      3b462ce5