1. 28 Feb, 2023 1 commit
    • uriyage's avatar
      Optimization: sdsRemoveFreeSpace to avoid realloc on noop (#11766) · af80a4a5
      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>
      (cherry picked from commit 46393f98)
      af80a4a5
  2. 16 Jan, 2023 1 commit
    • Oran Agra's avatar
      Fix potential issue with Lua argv caching, module command filter and libc realloc (#11652) · 61a1d454
      Oran Agra authored
      TLDR: solve a problem introduced in Redis 7.0.6 (#11541) with
      RM_CommandFilterArgInsert being called from scripts, which can
      lead to memory corruption.
      
      Libc realloc can return the same pointer even if the size was changed. The code in
      freeLuaRedisArgv had an assumption that if the pointer didn't change, then the
      allocation didn't change, and the cache can still be reused.
      However, if rewriteClientCommandArgument or RM_CommandFilterArgInsert were
      used, it could be that we realloced the argv array, and the pointer didn't change, then
      a consecutive command being executed from Lua can use that argv cache reaching
      beyond its size.
      This was actually only possible with modules, since the decision to realloc was based
      on argc, rather than argv_len.
      
      (cherry picked from commit c8052122)
      61a1d454
  3. 12 Dec, 2022 5 commits
    • uriyage's avatar
      Module CLIENT_CHANGE, Fix crash on free blocked client with DB!=0 (#11500) · 7ad786db
      uriyage authored
      
      
      In moduleFireServerEvent we change the real client DB to 0 on freeClient in case the event is REDISMODULE_EVENT_CLIENT_CHANGE.
      It results in a crash if the client is blocked on a key on other than DB 0.
      
      The DB change is not necessary even for module-client, as we set its DB to 0 on either createClient or moduleReleaseTempClient.
      Co-authored-by: default avatarMadelyn Olson <34459052+madolson@users.noreply.github.com>
      Co-authored-by: default avatarBinbin <binloveplay1314@qq.com>
      (cherry picked from commit e4eb18b3)
      7ad786db
    • sundb's avatar
      Fix crash due to to reuse iterator entry after list deletion in module (#11383) · 9fc20f4f
      sundb authored
      
      
      In the module, we will reuse the list iterator entry for RM_ListDelete, but `listTypeDelete` will only update
      `quicklistEntry->zi` but not `quicklistEntry->node`, which will result in `quicklistEntry->node` pointing to
      a freed memory address if the quicklist node is deleted.
      
      This PR sync `key->u.list.index` and `key->u.list.entry` to list iterator after `RM_ListDelete`.
      
      This PR also optimizes the release code of the original list iterator.
      Co-authored-by: default avatarViktor Söderqvist <viktor@zuiderkwast.se>
      (cherry picked from commit 6dd21355)
      9fc20f4f
    • Meir Shpilraien (Spielrein)'s avatar
      `RedisModule_ResetDataset` should not clear the functions. (#11268) · 6cf24fa4
      Meir Shpilraien (Spielrein) authored
      As mentioned on docs, `RM_ResetDataset` Performs similar operation to FLUSHALL.
      As FLUSHALL do not clean the function, `RM_ResetDataset` should not clean the functions
      as well.
      
      (cherry picked from commit d2ad01ab)
      6cf24fa4
    • guybe7's avatar
      RM_CreateCommand should not set CMD_KEY_VARIABLE_FLAGS automatically (#11320) · 5b2119c6
      guybe7 authored
      The original idea behind auto-setting the default (first,last,step) spec was to use
      the most "open" flags when the user didn't provide any key-spec flags information.
      
      While the above idea is a good approach, it really makes no sense to set
      CMD_KEY_VARIABLE_FLAGS if the user didn't provide the getkeys-api flag:
      in this case there's not way to retrieve these variable flags, so what's the point?
      
      Internally in redis there was code to ignore this already, so this fix doesn't change
      redis's behavior, it only affects the output of COMMAND command.
      
      (cherry picked from commit 3330ea18)
      5b2119c6
    • Shaya Potter's avatar
      Add RM_SetContextUser to support acl validation in RM_Call (and scripts) (#10966) · b8fcd322
      Shaya Potter authored
      Adds a number of user management/ACL validaiton/command execution functions to improve a
      Redis module's ability to enforce ACLs correctly and easily.
      
      * RM_SetContextUser - sets a RedisModuleUser on the context, which RM_Call will use to both
        validate ACLs (if requested and set) as well as assign to the client so that scripts executed via
        RM_Call will have proper ACL validation.
      * RM_SetModuleUserACLString - Enables one to pass an entire ACL string, not just a single OP
        and have it applied to the user
      * RM_GetModuleUserACLString - returns a stringified version of the user's ACL (same format as dump
        and list).  Contains an optimization to cache the stringified version until the underlying ACL is modified.
      * Slightly re-purpose the "C" flag to RM_Call from just being about ACL check before calling the
        command, to actually running the command with the right user, so that it also affects commands
        inside EVAL scripts. see #11231
      
      (cherry picked from commit 6e993a5d)
      b8fcd322
  4. 21 Sep, 2022 3 commits
    • Shaya Potter's avatar
      Improve cmd_flags for script/functions in RM_Call (#11159) · a1ec0cae
      Shaya Potter authored
      When RM_Call was used with `M` (reject OOM), `W` (reject writes),
      as well as `S` (rejecting stale or write commands in "Script mode"),
      it would have only checked the command flags, but not the declared
      script flag in case it's a command that runs a script.
      
      Refactoring: extracts out similar code in server.c's processCommand
      to be usable in RM_Call as well.
      
      (cherry picked from commit bed6d759)
      a1ec0cae
    • Binbin's avatar
      Fix memory leak in moduleFreeCommand (#11147) · fa60cd13
      Binbin authored
      Currently, we call zfree(cmd->args), but the argument array
      needs to be freed recursively (there might be sub-args).
      Also fixed memory leaks on cmd->tips and cmd->history.
      
      Fixes #11145
      
      (cherry picked from commit fc3956e8)
      fa60cd13
    • chenyang8094's avatar
      Register abs-expire apis (#11025) · c0394286
      chenyang8094 authored
      RM_SetAbsExpire and RM_GetAbsExpire were not actually operational since
      they were introduced, due to omission in API registration.
      
      (cherry picked from commit 39d216a3)
      c0394286
  5. 27 Jun, 2022 1 commit
    • Viktor Söderqvist's avatar
      Add missing REDISMODULE_CLIENTINFO_INITIALIZER (#10885) · 6af02100
      Viktor Söderqvist authored
      The module API docs mentions this macro, but it was not defined (so no one could have used it).
      
      Instead of adding it as is, we decided to add a _V1 macro, so that if / when we some day extend this struct,
      modules that use this API and don't need the extra fields, will still use the old version
      and still be compatible with older redis version (despite being compiled with newer redismodule.h)
      6af02100
  6. 26 Jun, 2022 2 commits
  7. 21 Jun, 2022 2 commits
    • Viktor Söderqvist's avatar
      Module API docs corrections (#10890) · 02acb8fd
      Viktor Söderqvist authored
      * Fix typo `RedisModule_CreatString` -> `RedisModule_CreateString` (multiple occurrences)
      * Make the markdown gen script change all `RM_` to `RedisModule_` even in code examples, etc.
      02acb8fd
    • Meir Shpilraien (Spielrein)'s avatar
      Fix crash on RM_Call with script mode. (#10886) · 61baabd8
      Meir Shpilraien (Spielrein) authored
      The PR fixes 2 issues:
      
      ### RM_Call crash on script mode
      
      `RM_Call` can potentially be called from a background thread where `server.current_client`
      are not set. In such case we get a crash on `NULL` dereference.
      The fix is to check first if `server.current_client` is `NULL`, if it does we should
      verify disc errors and readonly replica as we do to any normal clients (no masters nor AOF).
      
      ### RM_Call block OOM commands when not needed
      
      Again `RM_Call` can be executed on a background thread using a `ThreadSafeCtx`.
      In such case `server.pre_command_oom_state` can be irrelevant and should not be
      considered when check OOM state. This cause OOM commands to be blocked when
      not necessarily needed.
      
      In such case, check the actual used memory (and not the cached value). Notice that in
      order to know if the cached value can be used, we check that the ctx that was used on
      the `RM_Call` is a ThreadSafeCtx. Module writer can potentially abuse the API and use
      ThreadSafeCtx on the main thread. We consider this as a API miss used.
      61baabd8
  8. 12 Jun, 2022 1 commit
    • Binbin's avatar
      Fixed SET and BITFIELD commands being wrongly marked movablekeys (#10837) · 92fb4f4f
      Binbin authored
      
      
      The SET and BITFIELD command were added `get_keys_function` in #10148, causing
      them to be wrongly marked movablekeys in `populateCommandMovableKeys`.
      
      This was an unintended side effect introduced in #10148 (7.0 RC1)
      which could cause some clients an extra round trip for these commands in cluster mode.
      
      Since we define movablekeys as a way to determine if the legacy range [first, last, step]
      doesn't find all keys, then we need a completely different approach.
      
      The right approach should be to check if the legacy range covers all key-specs,
      and if none of the key-specs have the INCOMPLETE flag. 
      This way, we don't need to look at getkeys_proc of VARIABLE_FLAG at all.
      Probably with the exception of modules, who may still not be using key-specs.
      
      In this PR, we removed `populateCommandMovableKeys` and put its logic in
      `populateCommandLegacyRangeSpec`.
      In order to properly serve both old and new modules, we must probably keep relying
      CMD_MODULE_GETKEYS, but do that only for modules that don't declare key-specs. 
      For ones that do, we need to take the same approach we take with native redis commands.
      
      This approach was proposed by Oran. Fixes #10833
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      92fb4f4f
  9. 03 Jun, 2022 1 commit
  10. 01 Jun, 2022 1 commit
    • Oran Agra's avatar
      Fix broken protocol in MISCONF error, RM_Yield bugs, RM_Call(EVAL) OOM check... · b2061de2
      Oran Agra authored
      Fix broken protocol in MISCONF error, RM_Yield bugs, RM_Call(EVAL) OOM check bug, and new RM_Call checks. (#10786)
      
      * Fix broken protocol when redis can't persist to RDB (general commands, not
        modules), excessive newline. regression of #10372 (7.0 RC3)
      * Fix broken protocol when Redis can't persist to AOF (modules and
        scripts), missing newline.
      * Fix bug in OOM check of EVAL scripts called from RM_Call.
        set the cached OOM state for scripts before executing module commands too,
        so that it can serve scripts that are executed by modules.
        i.e. in the past EVAL executed by RM_Call could have either falsely
        fail or falsely succeeded because of a wrong cached OOM state flag.
      * Fix bugs with RM_Yield:
        1. SHUTDOWN should only accept the NOSAVE mode
        2. Avoid eviction during yield command processing.
        3. Avoid processing master client commands while yielding from another client
      * Add new two more checks to RM_Call script mode.
        1. READONLY You can't write against a read only replica
        2. MASTERDOWN Link with MASTER is down and `replica-serve-stale-data` is set to `no`
      * Add new RM_Call flag to let redis automatically refuse `deny-oom` commands
        while over the memory limit. 
      * Add tests to cover various errors from Scripts, Modules, Modules
        calling scripts, and Modules calling commands in script mode.
      
      Add tests:
      * Looks like the MISCONF error was completely uncovered by the tests,
        add tests for it, including from scripts, and modules
      * Add tests for NOREPLICAS from scripts
      * Add tests for the various errors in module RM_Call, including RM_Call that
        calls EVAL, and RM_call in "eval mode". that includes:
        NOREPLICAS, READONLY, MASTERDOWN, MISCONF
      b2061de2
  11. 31 May, 2022 1 commit
  12. 26 May, 2022 1 commit
  13. 18 May, 2022 1 commit
  14. 15 May, 2022 1 commit
  15. 10 May, 2022 1 commit
    • guybe7's avatar
      Dediacted member to hold RedisModuleCommand (#10681) · 815a6f84
      guybe7 authored
      Fix #10552
      
      We no longer piggyback getkeys_proc to hold the RedisModuleCommand struct, when exists
      
      Others:
      Use `doesCommandHaveKeys` in `RM_GetCommandKeysWithFlags` and `getKeysSubcommandImpl`.
      It causes a very minor behavioral change in commands that don't have actual keys, but have a spec
      with `CMD_KEY_NOT_KEY`.
      For example, before this command `COMMAND GETKEYS SPUBLISH` would return
      `Invalid arguments specified for command` but not it returns `The command has no key arguments`
      815a6f84
  16. 09 May, 2022 3 commits
  17. 26 Apr, 2022 1 commit
    • Oran Agra's avatar
      Add module API flag for using enum configs as bit flags (#10643) · 81926254
      Oran Agra authored
      Enables registration of an enum config that'll let the user pass multiple keywords that
      will be combined with `|` as flags into the integer config value.
      
      ```
          const char *enum_vals[] = {"none", "one", "two", "three"};
          const int int_vals[] = {0, 1, 2, 4};
      
          if (RedisModule_RegisterEnumConfig(ctx, "flags", 3, REDISMODULE_CONFIG_DEFAULT | REDISMODULE_CONFIG_BITFLAGS, enum_vals, int_vals, 4, getFlagsConfigCommand, setFlagsConfigCommand, NULL, NULL) == REDISMODULE_ERR) {
              return REDISMODULE_ERR;
          }
      ```
      doing:
      `config set moduleconfigs.flags "two three"` will result in 6 being passed to`setFlagsConfigCommand`.
      81926254
  18. 25 Apr, 2022 2 commits
  19. 20 Apr, 2022 1 commit
    • 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
  20. 18 Apr, 2022 1 commit
    • 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
  21. 17 Apr, 2022 2 commits
    • 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
    • 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
  22. 13 Apr, 2022 1 commit
    • 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
  23. 12 Apr, 2022 1 commit
  24. 11 Apr, 2022 1 commit
  25. 10 Apr, 2022 2 commits
    • guybe7's avatar
      Add RM_TryAlloc (#10541) · eeb0f142
      guybe7 authored
      Similarly to LCS, some modules would want to try to allocate memory, and
      fail gracefully if the allocation fails
      eeb0f142
    • guybe7's avatar
      COMMAND DOCS shows module name, where applicable (#10544) · 719db14e
      guybe7 authored
      Add field to COMMAND DOCS response to denote the name of the module
      that added that command.
      COMMAND LIST can filter by module, but if you get the full commands list,
      you may still wanna know which command belongs to which module.
      The alternative would be to do MODULE LIST, and then multiple calls to COMMAND LIST
      719db14e
  26. 07 Apr, 2022 1 commit
    • 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
  27. 05 Apr, 2022 1 commit