1. 08 Oct, 2024 1 commit
  2. 08 Sep, 2024 1 commit
    • Ozan Tezcan's avatar
      Fix flaky replication tests (#13518) · ac03e372
      Ozan Tezcan authored
      #13495 introduced a change to reply -LOADING while flushing existing db on a replica. Some of our tests are
       sensitive to this change and do no expect -LOADING reply.
      
      Fixing a couple of tests that fail time to time.
      ac03e372
  3. 16 Jul, 2024 1 commit
    • debing.sun's avatar
      Trigger Lua GC after script loading (#13407) · 88af96c7
      debing.sun authored
      Nowdays we do not trigger LUA GC after loading lua script. This means
      that when a large number of scripts are loaded, such as when functions
      are propagating from the master to the replica, if the LUA scripts are
      never touched on the replica, the garbage might remain there
      indefinitely.
      
      Before this PR, we would share a gc_count between scripts and functions.
      This means that, under certain circumstances, the GC trigger for scripts
      and functions was not fair.
      For example, loading a large number of scripts followed by a small
      number of functions could result in the functions triggering GC.
      In this PR, we assign a unique `gc_count` to each of them, so the GC
      triggers between them will no longer affect each other.
      
      on the other hand, this PR will to bring regession for script loading
      commands(`FUNCTION LOAD` and `SCRIPT LOAD`), but they are not hot path,
      we can ignore it, and it will be replaced
      https://github.com/redis/redis/pull/13375
      
       in the future.
      
      ---------
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      88af96c7
  4. 11 Jul, 2024 1 commit
  5. 13 Mar, 2024 1 commit
    • Binbin's avatar
      Lua eval scripts first in first out LRU eviction (#13108) · ad28d222
      Binbin authored
      In some cases, users will abuse lua eval. Each EVAL call generates
      a new lua script, which is added to the lua interpreter and cached
      to redis-server, consuming a large amount of memory over time.
      
      Since EVAL is mostly the one that abuses the lua cache, and these
      won't have pipeline issues (i.e. the script won't disappear
      unexpectedly,
      and cause errors like it would with SCRIPT LOAD and EVALSHA),
      we implement a plain FIFO LRU eviction only for these (not for
      scripts loaded with SCRIPT LOAD).
      
      ### Implementation notes:
      When not abused we'll probably have less than 100 scripts, and when
      abused we'll have many thousands. So we use a hard coded value of 500
      scripts. And considering that we don't have many scripts, then unlike
      keys, we don't need to worry about the memory usage of keeping a true
      sorted LRU linked list. We compute the SHA of each script anyway,
      and put the script in a dict, we can store a listNode there, and use
      it for quick removal and re-insertion into an LRU list each time the
      script is used.
      
      ### New interfaces:
      At the same time, a new `evicted_scripts` field is added to
      INFO, which represents the number of evicted eval scripts. Users
      can check it to see if they are abusing EVAL.
      
      ### benchmark:
      `./src/redis-benchmark -P 10 -n 1000000 -r 10000000000 eval "return
      __rand_int__" 0`
      
      The simple abuse of eval benchmark test that will create 1 million EVAL
      scripts. The performance has been improved by 50%, and the max latency
      has dropped from 500ms to 13ms (this may be caused by table expansion
      inside Lua when the number of scripts is large). And in the INFO memory,
      it used to consume 120MB (server cache) + 310MB (lua engine), but now
      it only consumes 70KB (server cache) + 210KB (lua_engine) because of
      the scripts eviction.
      
      For non-abusive case of about 100 EVAL scripts, there's no noticeable
      change in performance or memory usage.
      
      ### unlikely potentially breaking change:
      in theory, a user can maybe load a
      script with EVAL and then use EVALSHA to call it (by calculating the
      SHA1 value on the client side), it could be that if we read the docs
      carefully we'll realized it's a valid scenario, but we suppose it's
      extremely rare. So it may happen that EVALSHA acts on a script created
      by EVAL, and the script is evicted and EVALSHA returns a NOSCRIPT error.
      that is if you have more than 500 scripts being used in the same
      transaction / pipeline.
      
      This solves the second point in #13102.
      ad28d222
  6. 10 Mar, 2024 1 commit
    • Matthew Douglass's avatar
      Fix conversion of numbers in lua args to redis args (#13115) · 5fdaa53d
      Matthew Douglass authored
      
      
      Since lua_Number is not explicitly an integer or a double, we need to
      make an effort
      to convert it as an integer when that's possible, since the string could
      later be used
      in a context that doesn't support scientific notation (e.g. 1e9 instead
      of 100000000).
      
      Since fpconv_dtoa converts numbers with the equivalent of `%f` or `%e`,
      which ever is shorter,
      this would break if we try to pass a long integer number to a command
      that takes integer.
      we'll get an implicit conversion to string in Lua, and then the parsing
      in getLongLongFromObjectOrReply will fail.
      
      ```
      > eval "redis.call('hincrby', 'key', 'field', '1000000000')" 0
      (nil)
      > eval "redis.call('hincrby', 'key', 'field', tonumber('1000000000'))" 0
      (error) ERR value is not an integer or out of range script: ac99c32e4daf7e300d593085b611de261954a946, on @user_script:1.
      ```
      
      Switch to using ll2string if the number can be safely represented as a
      long long.
      
      The problem was introduced in #10587 (Redis 7.2).
      closes #13113.
      
      ---------
      Co-authored-by: default avatarBinbin <binloveplay1314@qq.com>
      Co-authored-by: default avatardebing.sun <debing.sun@redis.com>
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      5fdaa53d
  7. 22 Feb, 2024 1 commit
  8. 23 Jan, 2024 1 commit
    • Binbin's avatar
      Allow running WAITAOF in scripts, remove NOSCRIPT flag (#12977) · 85c31e0c
      Binbin authored
      In #11568 we removed the NOSCRIPT flag from commands, e.g. removing
      NOSCRIPT flag from WAIT. Aiming to allow them in scripts and let them
      implicitly behave in the non-blocking way.
      
      This PR remove NOSCRIPT flag from WAITAOF just like WAIT (to be
      symmetrical)).
      And this PR also add BLOCKING flag for WAIT and WAITAOF.
      85c31e0c
  9. 12 Oct, 2023 1 commit
    • zhaozhao.zz's avatar
      support XREAD[GROUP] with BLOCK option in scripts (#12596) · 77a65e82
      zhaozhao.zz authored
      In #11568 we removed the NOSCRIPT flag from commands and keep the BLOCKING flag.
      Aiming to allow them in scripts and let them implicitly behave in the non-blocking way.
      
      In that sense, the old behavior was to allow LPOP and reject BLPOP, and the new behavior,
      is to allow BLPOP too, and fail it only in case it ends up blocking.
      So likewise, so far we allowed XREAD and rejected XREAD BLOCK, and we will now allow
      that too, and only reject it if it ends up blocking.
      77a65e82
  10. 05 Aug, 2023 2 commits
    • zhaozhao.zz's avatar
      do not call handleClientsBlockedOnKeys inside yielding command (#12459) · 8226f39f
      zhaozhao.zz authored
      
      
      Fix the assertion when a busy script (timeout) signal ready keys (like LPUSH),
      and then an arbitrary client's `allow-busy` command steps into `handleClientsBlockedOnKeys`
      try wake up clients blocked on keys (like BLPOP).
      
      Reproduction process:
      1. start a redis with aof
          `./redis-server --appendonly yes`
      2. exec blpop
          `127.0.0.1:6379> blpop a 0`
      3. use another client call a busy script and this script push the blocked key
          `127.0.0.1:6379> eval "redis.call('lpush','a','b') while(1) do end" 0`
      4. user a new client call an allow-busy command like auth
          `127.0.0.1:6379> auth a`
      
      BTW, this issue also break the atomicity of script.
      
      This bug has been around for many years, the old versions only have the
      atomic problem, only 7.0/7.2 has the assertion problem.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      8226f39f
    • sundb's avatar
      Avoid mostly harmless integer overflow in cjson (#12456) · da9c2804
      sundb authored
      This PR mainly fixes a possible integer overflow in `json_append_string()`.
      When we use `cjson.encoding()` to encode a string larger than 2GB, at specific
      compilation flags, an integer overflow may occur leading to truncation, resulting
      in the part of the string larger than 2GB not being encoded.
      On the other hand, this overflow doesn't cause any read or write out-of-range or segment fault.
      
      1) using -O0 for lua_cjson (`make LUA_DEBUG=yes`)
          In this case, `i` will overflow and leads to truncation.
          When `i` reaches `INT_MAX+1` and overflows to INT_MIN, when compared to
          len, `i` (1000000..00) is expanded to 64 bits signed integer (1111111.....000000) .
          At this point i will be greater than len and jump out of the loop, so `for (i = 0; i < len; i++)`
          will loop up to 2^31 times, and the part of larger than 2GB will be truncated.
      
      ```asm
      `i` => -0x24(%rbp)
      <+253>:   addl   $0x1,-0x24(%rbp)       ; overflow if i large than 2^31
      <+257>:   mov    -0x24(%rbp),%eax
      <+260>:   movslq %eax,%rdx	            ; move a 32-bit value with sign extension into a 64-bit signed
      <+263>:   mov    -0x20(%rbp),%rax
      <+267>:   cmp    %rax,%rdx              ; check `i < len`
      <+270>:   jb     0x212600 <json_append_string+148>
      ```
         
      2) using -O2/-O3 for lua_cjson (`make LUA_DEBUG=no`, **the default**)
          In this case, because singed integer overflow is an undefined behavior, `i` will not overflow.
         `i` will be optimized by the compiler and use 64-bit registers for all subsequent instructions.
      
      ```asm
      <+180>:   add    $0x1,%rbx           ; Using 64-bit register `rbx` for i++
      <+184>:   lea    0x1(%rdx),%rsi
      <+188>:   mov    %rsi,0x10(%rbp)
      <+192>:   mov    %al,(%rcx,%rdx,1)
      <+195>:   cmp    %rbx,(%rsp)         ; check `i < len`
      <+199>:   ja     0x20b63a <json_append_string+154>
      ```
      
      3) using 32bit
          Because `strbuf_ensure_empty_length()` preallocates memory of length (len * 6 + 2),
          in 32-bit `cjson.encode()` can only handle strings smaller than ((2 ^ 32) - 3 ) / 6.
          So 32bit is not affected.
      
      Also change `i` in `strbuf_append_string()` to `size_t`.
      Since its second argument `str` is taken from the `char2escape` string array which is never
      larger than 6, so `strbuf_append_string()` is not at risk of overflow (the bug was unreachable).
      da9c2804
  11. 10 Jul, 2023 1 commit
  12. 16 Mar, 2023 1 commit
    • Meir Shpilraien (Spielrein)'s avatar
      Support for RM_Call on blocking commands (#11568) · d0da0a6a
      Meir Shpilraien (Spielrein) authored
      Allow running blocking commands from within a module using `RM_Call`.
      
      Today, when `RM_Call` is used, the fake client that is used to run command
      is marked with `CLIENT_DENY_BLOCKING` flag. This flag tells the command
      that it is not allowed to block the client and in case it needs to block, it must
      fallback to some alternative (either return error or perform some default behavior).
      For example, `BLPOP` fallback to simple `LPOP` if it is not allowed to block.
      
      All the commands must respect the `CLIENT_DENY_BLOCKING` flag (including
      module commands). When the command invocation finished, Redis asserts that
      the client was not blocked.
      
      This PR introduces the ability to call blocking command using `RM_Call` by
      passing a callback that will be called when the client will get unblocked.
      In order to do that, the user must explicitly say that he allow to perform blocking
      command by passing a new format specifier argument, `K`, to the `RM_Call`
      function. This new flag will tell Redis that it is allow to run blocking command
      and block the client. In case the command got blocked, Redis will return a new
      type of call reply (`REDISMODULE_REPLY_PROMISE`). This call reply indicates
      that the command got blocked and the user can set the on_unblocked handler using
      `RM_CallReplyPromiseSetUnblockHandler`.
      
      When clients gets unblocked, it eventually reaches `processUnblockedClients` function.
      This is where we check if the client is a fake module client and if it is, we call the unblock
      callback instead of performing the usual unblock operations.
      
      **Notice**: `RM_CallReplyPromiseSetUnblockHandler` must be called atomically
      along side the command invocation (without releasing the Redis lock in between).
      In addition, unlike other CallReply types, the promise call reply must be released
      by the module when the Redis GIL is acquired.
      
      The module can abort the execution on the blocking command (if it was not yet
      executed) using `RM_CallReplyPromiseAbort`. the API will return `REDISMODULE_OK`
      on success and `REDISMODULE_ERR` if the operation is already executed.
      **Notice** that in case of misbehave module, Abort might finished successfully but the
      operation will not really be aborted. This can only happened if the module do not respect
      the disconnect callback of the blocked client. 
      For pure Redis commands this can not happened.
      
      ### Atomicity Guarantees
      
      The API promise that the unblock handler will run atomically as an execution unit.
      This means that all the operation performed on the unblock handler will be wrapped
      with a multi exec transaction when replicated to the replica and AOF.
      The API **do not** grantee any other atomicity properties such as when the unblock
      handler will be called. This gives us the flexibility to strengthen the grantees (or not)
      in the future if we will decide that we need a better guarantees.
      
      That said, the implementation **does** provide a better guarantees when performing
      pure Redis blocking command like `BLPOP`. In this case the unblock handler will run
      atomically with the operation that got unblocked (for example, in case of `BLPOP`, the
      unblock handler will run atomically with the `LPOP` operation that run when the command
      got unblocked). This is an implementation detail that might be change in the future and the
      module writer should not count on that.
      
      ### Calling blocking commands while running on script mode (`S`)
      
      `RM_Call` script mode (`S`) was introduced on #0372. It is used for usecases where the
      command that was invoked on `RM_Call` comes from a user input and we want to make
      sure the user will not run dangerous commands like `shutdown`. Some command, such
      as `BLPOP`, are marked with `NO_SCRIPT` flag, which means they will not be allowed on
      script mode. Those commands are marked with  `NO_SCRIPT` just because they are
      blocking commands and not because they are dangerous. Now that we can run blocking
      commands on RM_Call, there is no real reason not to allow such commands on script mode.
      
      The underline problem is that the `NO_SCRIPT` flag is abused to also mark some of the
      blocking commands (notice that those commands know not to block the client if it is not
      allowed to do so, and have a fallback logic to such cases. So even if those commands
      were not marked with `NO_SCRIPT` flag, it would not harm Redis, and today we can
      already run those commands within multi exec).
      
      In addition, not all blocking commands are marked with `NO_SCRIPT` flag, for example
      `blmpop` are not marked and can run from within a script.
      
      Those facts shows that there are some ambiguity about the meaning of the `NO_SCRIPT`
      flag, and its not fully clear where it should be use.
      
      The PR suggest that blocking commands should not be marked with `NO_SCRIPT` flag,
      those commands should handle `CLIENT_DENY_BLOCKING` flag and only block when
      it's safe (like they already does today). To achieve that, the PR removes the `NO_SCRIPT`
      flag from the following commands:
      * `blmove`
      * `blpop`
      * `brpop`
      * `brpoplpush`
      * `bzpopmax`
      * `bzpopmin`
      * `wait`
      
      This might be considered a breaking change as now, on scripts, instead of getting
      `command is not allowed from script` error, the user will get some fallback behavior
      base on the command implementation. That said, the change matches the behavior
      of scripts and multi exec with respect to those commands and allow running them on
      `RM_Call` even when script mode is used.
      
      ### Additional RedisModule API and changes
      
      * `RM_BlockClientSetPrivateData` - Set private data on the blocked client without the
        need to unblock the client. This allows up to set the promise CallReply as the private
        data of the blocked client and abort it if the client gets disconnected.
      * `RM_BlockClientGetPrivateData` - Return the current private data set on a blocked client.
        We need it so we will have access to this private data on the disconnect callback.
      * On RM_Call, the returned reply will be added to the auto memory context only if auto
        memory is enabled, this allows us to keep the call reply for longer time then the context
        lifetime and does not force an unneeded borrow relationship between the CallReply and
        the RedisModuleContext.
      d0da0a6a
  13. 11 Mar, 2023 1 commit
    • guybe7's avatar
      Add reply_schema to command json files (internal for now) (#10273) · 4ba47d2d
      guybe7 authored
      Work in progress towards implementing a reply schema as part of COMMAND DOCS, see #9845
      Since ironing the details of the reply schema of each and every command can take a long time, we
      would like to merge this PR when the infrastructure is ready, and let this mature in the unstable branch.
      Meanwhile the changes of this PR are internal, they are part of the repo, but do not affect the produced build.
      
      ### Background
      In #9656 we add a lot of information about Redis commands, but we are missing information about the replies
      
      ### Motivation
      1. Documentation. This is the primary goal.
      2. It should be possible, based on the output of COMMAND, to be able to generate client code in typed
        languages. In order to do that, we need Redis to tell us, in detail, what each reply looks like.
      3. We would like to build a fuzzer that verifies the reply structure (for now we use the existing
        testsuite, see the "Testing" section)
      
      ### Schema
      The idea is to supply some sort of schema for the various replies of each command.
      The schema will describe the conceptual structure of the reply (for generated clients), as defined in RESP3.
      Note that the reply structure itself may change, depending on the arguments (e.g. `XINFO STREAM`, with
      and without the `FULL` modifier)
      We decided to use the standard json-schema (see https://json-schema.org/) as the reply-schema.
      
      Example for `BZPOPMIN`:
      ```
      "reply_schema": {
          "oneOf": [
              {
                  "description": "Timeout reached and no elements were popped.",
                  "type": "null"
              },
              {
                  "description": "The keyname, popped member, and its score.",
                  "type": "array",
                  "minItems": 3,
                  "maxItems": 3,
                  "items": [
                      {
                          "description": "Keyname",
                          "type": "string"
                      },
                      {
                          "description": "Member",
                          "type": "string"
                      },
                      {
                          "description": "Score",
                          "type": "number"
                      }
                  ]
              }
          ]
      }
      ```
      
      #### Notes
      1.  It is ok that some commands' reply structure depends on the arguments and it's the caller's responsibility
        to know which is the relevant one. this comes after looking at other request-reply systems like OpenAPI,
        where the reply schema can also be oneOf and the caller is responsible to know which schema is the relevant one.
      2. The reply schemas will describe RESP3 replies only. even though RESP3 is structured, we want to use reply
        schema for documentation (and possibly to create a fuzzer that validates the replies)
      3. For documentation, the description field will include an explanation of the scenario in which the reply is sent,
        including any relation to arguments. for example, for `ZRANGE`'s two schemas we will need to state that one
        is with `WITHSCORES` and the other is without.
      4. For documentation, there will be another optional field "notes" in which we will add a short description of
        the representation in RESP2, in case it's not trivial (RESP3's `ZRANGE`'s nested array vs. RESP2's flat
        array, for example)
      
      Given the above:
      1. We can generate the "return" section of all commands in [redis-doc](https://redis.io/commands/)
        (given that "description" and "notes" are comprehensive enough)
      2. We can generate a client in a strongly typed language (but the return type could be a conceptual
        `union` and the caller needs to know which schema is relevant). see the section below for RESP2 support.
      3. We can create a fuzzer for RESP3.
      
      ### Limitations (because we are using the standard json-schema)
      The problem is that Redis' replies are more diverse than what the json format allows. This means that,
      when we convert the reply to a json (in order to validate the schema against it), we lose information (see
      the "Testing" section below).
      The other option would have been to extend the standard json-schema (and json format) to include stuff
      like sets, bulk-strings, error-string, etc. but that would mean also extending the schema-validator - and that
      seemed like too much work, so we decided to compromise.
      
      Examples:
      1. We cannot tell the difference between an "array" and a "set"
      2. We cannot tell the difference between simple-string and bulk-string
      3. we cannot verify true uniqueness of items in commands like ZRANGE: json-schema doesn't cover the
        case of two identical members with different scores (e.g. `[["m1",6],["m1",7]]`) because `uniqueItems`
        compares (member,score) tuples and not just the member name. 
      
      ### Testing
      This commit includes some changes inside Redis in order to verify the schemas (existing and future ones)
      are indeed correct (i.e. describe the actual response of Redis).
      To do that, we added a debugging feature to Redis that causes it to produce a log of all the commands
      it executed and their replies.
      For that, Redis needs to be compiled with `-DLOG_REQ_RES` and run with
      `--reg-res-logfile <file> --client-default-resp 3` (the testsuite already does that if you run it with
      `--log-req-res --force-resp3`)
      You should run the testsuite with the above args (and `--dont-clean`) in order to make Redis generate
      `.reqres` files (same dir as the `stdout` files) which contain request-response pairs.
      These files are later on processed by `./utils/req-res-log-validator.py` which does:
      1. Goes over req-res files, generated by redis-servers, spawned by the testsuite (see logreqres.c)
      2. For each request-response pair, it validates the response against the request's reply_schema
        (obtained from the extended COMMAND DOCS)
      5. In order to get good coverage of the Redis commands, and all their different replies, we chose to use
        the existing redis test suite, rather than attempt to write a fuzzer.
      
      #### Notes about RESP2
      1. We will not be able to use the testing tool to verify RESP2 replies (we are ok with that, it's time to
        accept RESP3 as the future RESP)
      2. Since the majority of the test suite is using RESP2, and we want the server to reply with RESP3
        so that we can validate it, we will need to know how to convert the actual reply to the one expected.
         - number and boolean are always strings in RESP2 so the conversion is easy
         - objects (maps) are always a flat array in RESP2
         - others (nested array in RESP3's `ZRANGE` and others) will need some special per-command
           handling (so the client will not be totally auto-generated)
      
      Example for ZRANGE:
      ```
      "reply_schema": {
          "anyOf": [
              {
                  "description": "A list of member elements",
                  "type": "array",
                  "uniqueItems": true,
                  "items": {
                      "type": "string"
                  }
              },
              {
                  "description": "Members and their scores. Returned in case `WITHSCORES` was used.",
                  "notes": "In RESP2 this is returned as a flat array",
                  "type": "array",
                  "uniqueItems": true,
                  "items": {
                      "type": "array",
                      "minItems": 2,
                      "maxItems": 2,
                      "items": [
                          {
                              "description": "Member",
                              "type": "string"
                          },
                          {
                              "description": "Score",
                              "type": "number"
                          }
                      ]
                  }
              }
          ]
      }
      ```
      
      ### Other changes
      1. Some tests that behave differently depending on the RESP are now being tested for both RESP,
        regardless of the special log-req-res mode ("Pub/Sub PING" for example)
      2. Update the history field of CLIENT LIST
      3. Added basic tests for commands that were not covered at all by the testsuite
      
      ### TODO
      
      - [x] (maybe a different PR) add a "condition" field to anyOf/oneOf schemas that refers to args. e.g.
        when `SET` return NULL, the condition is `arguments.get||arguments.condition`, for `OK` the condition
        is `!arguments.get`, and for `string` the condition is `arguments.get` - https://github.com/redis/redis/issues/11896
      - [x] (maybe a different PR) also run `runtest-cluster` in the req-res logging mode
      - [x] add the new tests to GH actions (i.e. compile with `-DLOG_REQ_RES`, run the tests, and run the validator)
      - [x] (maybe a different PR) figure out a way to warn about (sub)schemas that are uncovered by the output
        of the tests - https://github.com/redis/redis/issues/11897
      - [x] (probably a separate PR) add all missing schemas
      - [x] check why "SDOWN is triggered by misconfigured instance replying with errors" fails with --log-req-res
      - [x] move the response transformers to their own file (run both regular, cluster, and sentinel tests - need to
        fight with the tcl including mechanism a bit)
      - [x] issue: module API - https://github.com/redis/redis/issues/11898
      - [x] (probably a separate PR): improve schemas: add `required` to `object`s - https://github.com/redis/redis/issues/11899
      
      Co-authored-by: default avatarOzan Tezcan <ozantezcan@gmail.com>
      Co-authored-by: default avatarHanna Fadida <hanna.fadida@redislabs.com>
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      Co-authored-by: default avatarShaya Potter <shaya@redislabs.com>
      4ba47d2d
  14. 28 Feb, 2023 1 commit
    • uriyage's avatar
      Try to trim strings only when applicable (#11817) · 9d336ac3
      uriyage authored
      
      
      As `sdsRemoveFreeSpace` have an impact on performance even if it is a no-op (see details at #11508). 
      Only call the function when there is a possibility that the string contains free space.
      * For strings coming from the network, it's only if they're bigger than PROTO_MBULK_BIG_ARG
      * For strings coming from scripts, it's only if they're smaller than LUA_CMD_OBJCACHE_MAX_LEN
      * For strings coming from modules, it could be anything.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      Co-authored-by: default avatarsundb <sundbcn@gmail.com>
      9d336ac3
  15. 04 Jan, 2023 1 commit
    • Oran Agra's avatar
      Fix potential issue with Lua argv caching, module command filter and libc realloc (#11652) · c8052122
      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.
      c8052122
  16. 05 Dec, 2022 1 commit
    • 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
  17. 09 Oct, 2022 1 commit
    • Binbin's avatar
      Freeze time sampling during command execution, and scripts (#10300) · 35b3fbd9
      Binbin authored
      Freeze time during execution of scripts and all other commands.
      This means that a key is either expired or not, and doesn't change
      state during a script execution. resolves #10182
      
      This PR try to add a new `commandTimeSnapshot` function.
      The function logic is extracted from `keyIsExpired`, but the related
      calls to `fixed_time_expire` and `mstime()` are removed, see below.
      
      In commands, we will avoid calling `mstime()` multiple times
      and just use the one that sampled in call. The background is,
      e.g. using `PEXPIRE 1` with valgrind sometimes result in the key
      being deleted rather than expired. The reason is that both `PEXPIRE`
      command and `checkAlreadyExpired` call `mstime()` separately.
      
      There are other more important changes in this PR:
      1. Eliminate `fixed_time_expire`, it is no longer needed. 
         When we want to sample time we should always use a time snapshot. 
         We will use `in_nested_call` instead to update the cached time in `call`.
      2. Move the call for `updateCachedTime` from `serverCron` to `afterSleep`.
          Now `commandTimeSnapshot` will always return the sample time, the
          `lookupKeyReadWithFlags` call in `getNodeByQuery` will get a outdated
          cached time (because `processCommand` is out of the `call` context).
          We put the call to `updateCachedTime` in `aftersleep`.
      3. Cache the time each time the module lock Redis.
          Call `updateCachedTime` in `moduleGILAfterLock`, affecting `RM_ThreadSafeContextLock`
          and `RM_ThreadSafeContextTryLock`
      
      Currently the commandTimeSnapshot change affects the following TTL commands:
      - SET EX / SET PX
      - EXPIRE / PEXPIRE
      - SETEX / PSETEX
      - GETEX EX / GETEX PX
      - TTL / PTTL
      - EXPIRETIME / PEXPIRETIME
      - RESTORE key TTL
      
      And other commands just use the cached mstime (including TIME).
      
      This is considered to be a breaking change since it can break a script
      that uses a loop to wait for a key to expire.
      35b3fbd9
  18. 16 Aug, 2022 1 commit
  19. 07 Aug, 2022 1 commit
  20. 26 Jul, 2022 1 commit
    • Meir Shpilraien (Spielrein)'s avatar
      Fix #11030, use lua_rawget to avoid triggering metatables and crash. (#11032) · 020e046b
      Meir Shpilraien (Spielrein) authored
      Fix #11030, use lua_rawget to avoid triggering metatables.
      
      #11030 shows how return `_G` from the Lua script (either function or eval), cause the
      Lua interpreter to Panic and the Redis processes to exit with error code 1.
      Though return `_G` only panic on Redis 7 and 6.2.7, the underline issue exists on older
      versions as well (6.0 and 6.2). The underline issue is returning a table with a metatable
      such that the metatable raises an error.
      
      The following example demonstrate the issue:
      ```
      127.0.0.1:6379> eval "local a = {}; setmetatable(a,{__index=function() foo() end}) return a" 0
      Error: Server closed the connection
      ```
      ```
      PANIC: unprotected error in call to Lua API (user_script:1: Script attempted to access nonexistent global variable 'foo')
      ```
      
      The Lua panic happened because when returning the result to the client, Redis needs to
      introspect the returning table and transform the table into a resp. In order to scan the table,
      Redis uses `lua_gettable` api which might trigger the metatable (if exists) and might raise an error.
      This code is not running inside `pcall` (Lua protected call), so raising an error causes the
      Lua to panic and exit. Notice that this is not a crash, its a Lua panic that exit with error code 1.
      
      Returning `_G` panics on Redis 7 and 6.2.7 because on those versions `_G` has a metatable
      that raises error when trying to fetch a none existing key.
      
      ### Solution
      
      Instead of using `lua_gettable` that might raise error and cause the issue, use `lua_rawget`
      that simply return the value from the table without triggering any metatable logic.
      This is promised not to raise and error.
      
      The downside of this solution is that it might be considered as breaking change, if someone
      rely on metatable in the returned value. An alternative solution is to wrap this entire logic
      with `pcall` (Lua protected call), this alternative require a much bigger refactoring.
      
      ### Back Porting
      
      The same fix will work on older versions as well (6.2, 6.0). Notice that on those version,
      the issue can cause Redis to crash if inside the metatable logic there is an attempt to accesses
      Redis (`redis.call`). On 7.0, there is not crash and the `redis.call` is executed as if it was done
      from inside the script itself.
      
      ### Tests
      
      Tests was added the verify the fix
      020e046b
  21. 14 Jun, 2022 2 commits
    • Oran Agra's avatar
      Script that made modification will not break with unexpected NOREPLICAS error (#10855) · 8ef4f1db
      Oran Agra authored
      If a script made a modification and then was interrupted for taking too long.
      there's a chance redis will detect that a replica dropped and would like to reject
      write commands with NOREPLICAS due to insufficient good replicas.
      returning an error on a command in this case breaks the script atomicity.
      
      The same could in theory happen with READONLY, MISCONF, but i don't think
      these state changes can happen during script execution.
      8ef4f1db
    • Oran Agra's avatar
      Allow ECHO in loading and stale modes (#10853) · ffa00770
      Oran Agra authored
      I noticed that scripting.tcl uses INFO from within a script and thought it's an
      overkill and concluded it's nicer to use another CMD_STALE command,
      decided to use ECHO, and then noticed it's not at all allowed in stale mode.
      probably overlooked at #6843
      ffa00770
  22. 01 Jun, 2022 2 commits
    • Oran Agra's avatar
      Expose script flags to processCommand for better handling (#10744) · df558618
      Oran Agra authored
      The important part is that read-only scripts (not just EVAL_RO
      and FCALL_RO, but also ones with `no-writes` executed by normal EVAL or
      FCALL), will now be permitted to run during CLIENT PAUSE WRITE (unlike
      before where only the _RO commands would be processed).
      
      Other than that, some errors like OOM, READONLY, MASTERDOWN are now
      handled by processCommand, rather than the command itself affects the
      error string (and even error code in some cases), and command stats.
      
      Besides that, now the `may-replicate` commands, PFCOUNT and PUBLISH, will
      be considered `write` commands in scripts and will be blocked in all
      read-only scripts just like other write commands.
      They'll also be blocked in EVAL_RO (i.e. even for scripts without the
      `no-writes` shebang flag.
      
      This commit also hides the `may_replicate` flag from the COMMAND command
      output. this is a **breaking change**.
      
      background about may_replicate:
      We don't want to expose a no...
      df558618
    • 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
  23. 22 May, 2022 1 commit
    • Oran Agra's avatar
      Scripts that declare the `no-writes` flag are implicitly `allow-oom` too. (#10699) · b0e18f80
      Oran Agra authored
      Scripts that have the `no-writes` flag, cannot execute write commands,
      and since all `deny-oom` commands are write commands, we now act
      as if the `allow-oom` flag is implicitly set for scripts that set the `no-writes` flag.
      this also implicitly means that the EVAL*_RO and FCALL_RO commands can
      never fails with OOM error.
      
      Note about a bug that's no longer relevant:
      There was an issue with EVAL*_RO using shebang not being blocked correctly
      in OOM state:
      When an EVAL script declares a shebang, it was by default not allowed to run in
      OOM state.
      but this depends on a flag that is updated before the command is executed, which
      was not updated in case of the `_RO` variants.
      the result is that if the previous cached state was outdated (either true or false),
      the script will either unjustly fail with OOM, or unjustly allowed to run despite
      the OOM state.
      It doesn't affect scripts without a shebang since these depend on the actual
      commands they run, and since these are only read commands, they don't care
      for that cached oom state flag.
      it did affect scripts with shebang and no allow-oom flag, bug after the change in
      this PR, scripts that are run with eval_ro would implicitly have that flag so again
      the cached state doesn't matter.
      
      p.s. this isn't a breaking change since all it does is allow scripts to run when they
      should / could rather than blocking them.
      b0e18f80
  24. 26 Apr, 2022 2 commits
    • meir's avatar
      Protect any table which is reachable from globals and added globals white list. · efa162bc
      meir authored
      The white list is done by setting a metatable on the global table before initializing
      any library. The metatable set the `__newindex` field to a function that check
      the white list before adding the field to the table. Fields which is not on the
      white list are simply ignored.
      
      After initialization phase is done we protect the global table and each table
      that might be reachable from the global table. For each table we also protect
      the table metatable if exists.
      efa162bc
    • meir's avatar
      Protect globals of both evals scripts and functions. · 3731580b
      meir authored
      Use the new `lua_enablereadonlytable` Lua API to protect the global tables of
      both evals scripts and functions. For eval scripts, the implemetation is easy,
      We simply call `lua_enablereadonlytable` on the global table to turn it into
      a readonly table.
      
      On functions its more complecated, we want to be able to switch globals between
      load run and function run. To achieve this, we create a new empty table that
      acts as the globals table for function, we control the actual globals using metatable
      manipulation. Notice that even if the user gets a pointer to the original tables, all
      the tables are set to be readonly (using `lua_enablereadonlytable` Lua API) so he can
      not change them. The following inlustration better explain the solution:
      
      ```
      Global table {} <- global table metatable {.__index = __real_globals__}
      ```
      
      The `__real_globals__` is set depends on the run context (function load or function call).
      
      Why this solution is needed and its not enough to simply switch globals?
      When we run in the context of function load and create our functions, our function gets
      the current globals that was set when they were created. Replacing the globals after
      the creation will not effect them. This is why this trick it mandatory.
      3731580b
  25. 19 Apr, 2022 1 commit
  26. 17 Apr, 2022 1 commit
  27. 14 Apr, 2022 1 commit
    • Madelyn Olson's avatar
      Fix incorrect error code for eval scripts and fix test error checking (#10575) · effa707e
      Madelyn Olson authored
      By the convention of errors, there is supposed to be a space between the code and the name.
      While looking at some lua stuff I noticed that interpreter errors were not adding the space,
      so some clients will try to map the detailed error message into the error.
      
      We have tests that hit this condition, but they were just checking that the string "starts" with ERR.
      I updated some other tests with similar incorrect string checking. This isn't complete though, as
      there are other ways we check for ERR I didn't fix.
      
      Produces some fun output like:
      ```
      # Errorstats
      errorstat_ERR:count=1
      errorstat_ERRuser_script_1_:count=1
      ```
      effa707e
  28. 05 Apr, 2022 1 commit
    • Meir Shpilraien (Spielrein)'s avatar
      Functions: Move library meta data to be part of the library payload. (#10500) · ae020e3d
      Meir Shpilraien (Spielrein) authored
      ## Move library meta data to be part of the library payload.
      
      Following the discussion on https://github.com/redis/redis/issues/10429 and the intention to add (in the future) library versioning support, we believe that the entire library metadata (like name and engine) should be part of the library payload and not provided by the `FUNCTION LOAD` command. The reasoning behind this is that the programmer who developed the library should be the one who set those values (name, engine, and in the future also version). **It is not the responsibility of the admin who load the library into the database.**
      
      The PR moves all the library metadata (engine and function name) to be part of the library payload. The metadata needs to be provided on the first line of the payload using the shebang format (`#!<engine> name=<name>`), example:
      
      ```lua
      #!lua name=test
      redis.register_function('foo', function() return 1 end)
      ```
      
      The above script will run on the Lua engine and will create a library called `test`.
      
      ## API Changes (compare to 7.0 rc2)
      
      * `FUNCTION LOAD` command was change and now it simply gets the library payload and extract the engine and name from the payload. In addition, the command will now return the function name which can later be used on `FUNCTION DELETE` and `FUNCTION LIST`.
      * The description field was completely removed from`FUNCTION LOAD`, and `FUNCTION LIST`
      
      
      ## Breaking Changes (compare to 7.0 rc2)
      
      * Library description was removed (we can re-add it in the future either as part of the shebang line or an additional line).
      * Loading an AOF file that was generated by either 7.0 rc1 or 7.0 rc2 will fail because the old command syntax is invalid.
      
      ## Notes
      
      * Loading an RDB file that was generated by rc1 / rc2 **is** supported, Redis will automatically add the shebang to the libraries payloads (we can probably delete that code after 7.0.3 or so since there's no need to keep supporting upgrades from an RC build).
      ae020e3d
  29. 04 Apr, 2022 1 commit
  30. 27 Feb, 2022 1 commit
    • Meir Shpilraien (Spielrein)'s avatar
      Sort out the mess around Lua error messages and error stats (#10329) · aa856b39
      Meir Shpilraien (Spielrein) authored
      
      
      This PR fix 2 issues on Lua scripting:
      * Server error reply statistics (some errors were counted twice).
      * Error code and error strings returning from scripts (error code was missing / misplaced).
      
      ## Statistics
      a Lua script user is considered part of the user application, a sophisticated transaction,
      so we want to count an error even if handled silently by the script, but when it is
      propagated outwards from the script we don't wanna count it twice. on the other hand,
      if the script decides to throw an error on its own (using `redis.error_reply`), we wanna
      count that too.
      Besides, we do count the `calls` in command statistics for the commands the script calls,
      we we should certainly also count `failed_calls`.
      So when a simple `eval "return redis.call('set','x','y')" 0` fails, it should count the failed call
      to both SET and EVAL, but the `errorstats` and `total_error_replies` should be counted only once.
      
      The PR changes the error object that is raised on errors. Instead of raising a simple Lua
      string, Redis will raise a Lua table in the following format:
      
      ```
      {
          err='<error message (including error code)>',
          source='<User source file name>',
          line='<line where the error happned>',
          ignore_error_stats_update=true/false,
      }
      ```
      
      The `luaPushError` function was modified to construct the new error table as describe above.
      The `luaRaiseError` was renamed to `luaError` and is now simply called `lua_error` to raise
      the table on the top of the Lua stack as the error object.
      The reason is that since its functionality is changed, in case some Redis branch / fork uses it,
      it's better to have a compilation error than a bug.
      
      The `source` and `line` fields are enriched by the error handler (if possible) and the
      `ignore_error_stats_update` is optional and if its not present then the default value is `false`.
      If `ignore_error_stats_update` is true, the error will not be counted on the error stats.
      
      When parsing Redis call reply, each error is translated to a Lua table on the format describe
      above and the `ignore_error_stats_update` field is set to `true` so we will not count errors
      twice (we counted this error when we invoke the command).
      
      The changes in this PR might have been considered as a breaking change for users that used
      Lua `pcall` function. Before, the error was a string and now its a table. To keep backward
      comparability the PR override the `pcall` implementation and extract the error message from
      the error table and return it.
      
      Example of the error stats update:
      
      ```
      127.0.0.1:6379> lpush l 1
      (integer) 2
      127.0.0.1:6379> eval "return redis.call('get', 'l')" 0
      (error) WRONGTYPE Operation against a key holding the wrong kind of value. script: e471b73f1ef44774987ab00bdf51f21fd9f7974a, on @user_script:1.
      
      127.0.0.1:6379> info Errorstats
      # Errorstats
      errorstat_WRONGTYPE:count=1
      
      127.0.0.1:6379> info commandstats
      # Commandstats
      cmdstat_eval:calls=1,usec=341,usec_per_call=341.00,rejected_calls=0,failed_calls=1
      cmdstat_info:calls=1,usec=35,usec_per_call=35.00,rejected_calls=0,failed_calls=0
      cmdstat_lpush:calls=1,usec=14,usec_per_call=14.00,rejected_calls=0,failed_calls=0
      cmdstat_get:calls=1,usec=10,usec_per_call=10.00,rejected_calls=0,failed_calls=1
      ```
      
      ## error message
      We can now construct the error message (sent as a reply to the user) from the error table,
      so this solves issues where the error message was malformed and the error code appeared
      in the middle of the error message:
      
      ```diff
      127.0.0.1:6379> eval "return redis.call('set','x','y')" 0
      -(error) ERR Error running script (call to 71e6319f97b0fe8bdfa1c5df3ce4489946dda479): @user_script:1: OOM command not allowed when used memory > 'maxmemory'.
      +(error) OOM command not allowed when used memory > 'maxmemory' @user_script:1. Error running script (call to 71e6319f97b0fe8bdfa1c5df3ce4489946dda479)
      ```
      
      ```diff
      127.0.0.1:6379> eval "redis.call('get', 'l')" 0
      -(error) ERR Error running script (call to f_8a705cfb9fb09515bfe57ca2bd84a5caee2cbbd1): @user_script:1: WRONGTYPE Operation against a key holding the wrong kind of value
      +(error) WRONGTYPE Operation against a key holding the wrong kind of value script: 8a705cfb9fb09515bfe57ca2bd84a5caee2cbbd1, on @user_script:1.
      ```
      
      Notica that `redis.pcall` was not change:
      ```
      127.0.0.1:6379> eval "return redis.pcall('get', 'l')" 0
      (error) WRONGTYPE Operation against a key holding the wrong kind of value
      ```
      
      
      ## other notes
      Notice that Some commands (like GEOADD) changes the cmd variable on the client stats so we
      can not count on it to update the command stats. In order to be able to update those stats correctly
      we needed to promote `realcmd` variable to be located on the client struct.
      
      Tests was added and modified to verify the changes.
      
      Related PR's: #10279, #10218, #10278, #10309
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      aa856b39
  31. 08 Feb, 2022 1 commit
    • yoav-steinberg's avatar
      Consistent erros returned from EVAL scripts (#10218) · b76016a9
      yoav-steinberg authored
      This PR handles inconsistencies in errors returned from lua scripts.
      Details of the problem can be found in #10165.
      
      ### Changes
      
      - Remove double stack trace. It's enough that a stack trace is automatically added by the engine's error handler
        see https://github.com/redis/redis/blob/d0bc4fff18afdf9e5421cc88e23ffbb876ecaec3/src/function_lua.c#L472-L485
        and https://github.com/redis/redis/blob/d0bc4fff18afdf9e5421cc88e23ffbb876ecaec3/src/eval.c#L243-L255
      - Make sure all errors a preceded with an error code. Passing a simple string to `luaPushError()` will prepend it
        with a generic `ERR` error code.
      - Make sure lua error table doesn't include a RESP `-` error status. Lua stores redis error's as a lua table with a
        single `err` field and a string. When the string is translated back to RESP we add a `-` to it.
        See https://github.com/redis/redis/blob/d0bc4fff18afdf9e5421cc88e23ffbb876ecaec3/src/script_lua.c#L510-L517
        So there's no need to store it in the lua table.
      
      ### Before & After
      ```diff
      --- <unnamed>
      +++ <unnamed>
      @@ -1,14 +1,14 @@
        1: config set maxmemory 1
        2: +OK
        3: eval "return redis.call('set','x','y')" 0
      - 4: -ERR Error running script (call to 71e6319f97b0fe8bdfa1c5df3ce4489946dda479): @user_script:1: @user_script: 1: -OOM command not allowed when used memory > 'maxmemory'.
      + 4: -ERR Error running script (call to 71e6319f97b0fe8bdfa1c5df3ce4489946dda479): @user_script:1: OOM command not allowed when used memory > 'maxmemory'.
        5: eval "return redis.pcall('set','x','y')" 0
      - 6: -@user_script: 1: -OOM command not allowed when used memory > 'maxmemory'.
      + 6: -OOM command not allowed when used memory > 'maxmemory'.
        7: eval "return redis.call('select',99)" 0
        8: -ERR Error running script (call to 4ad5abfc50bbccb484223905f9a16f09cd043ba8): @user_script:1: ERR DB index is out of range
        9: eval "return redis.pcall('select',99)" 0
       10: -ERR DB index is out of range
       11: eval_ro "return redis.call('set','x','y')" 0
      -12: -ERR Error running script (call to 71e6319f97b0fe8bdfa1c5df3ce4489946dda479): @user_script:1: @user_script: 1: Write commands are not allowed from read-only scripts.
      +12: -ERR Error running script (call to 71e6319f97b0fe8bdfa1c5df3ce4489946dda479): @user_script:1: ERR Write commands are not allowed from read-only scripts.
       13: eval_ro "return redis.pcall('set','x','y')" 0
      -14: -@user_script: 1: Write commands are not allowed from read-only scripts.
      +14: -ERR Write commands are not allowed from read-only scripts.
      ```
      b76016a9
  32. 07 Feb, 2022 1 commit
    • yoav-steinberg's avatar
      acl check api for functions and eval (#10220) · 9dfeda58
      yoav-steinberg authored
      Changes:
      1. Adds the `redis.acl_check_cmd()` api to lua scripts. It can be used to check if the
        current user has permissions to execute a given command. The new function receives
        the command to check as an argument exactly like `redis.call()` receives the command
        to execute as an argument.
      2. In the PR I unified the code used to convert lua arguments to redis argv arguments from
        both the new `redis.acl_check_cmd()` API and the `redis.[p]call()` API. This cleans up
        potential duplicate code.
      3. While doing the refactoring in 2 I noticed there's an optimization to reduce allocation calls
        when parsing lua arguments into an `argv` array in the `redis.[p]call()` implementation.
        These optimizations were introduced years ago in 48c49c48
        and 4f686555. It is unclear why this was added.
        The original commit message claims a 4% performance increase which I couldn't recreate
        and might not be worth it even if it did recreate. This PR removes that optimization.
        Following are details of the benchmark I did that couldn't reveal any performance
        improvements due to this optimization:
      
      ```
      benchmark 1: src/redis-benchmark -P 500 -n 10000000 eval 'return redis.call("ping")' 0
      benchmark 2: src/redis-benchmark -P 500 -r 1000 -n 1000000 eval 'return redis.call("mset","k1__rand_int__","v1__rand_int__","k2__rand_int__","v2__rand_int__","k3__rand_int__","v3__rand_int__","k4__rand_int__","v4__rand_int__")' 0
      benchmark 3: src/redis-benchmark -P 500 -r 1000 -n 100000 eval "for i=1,100,1 do redis.call('set','kk'..i,'vv'..__rand_int__) end return redis.call('get','kk5')" 0
      benchmark 4: src/redis-benchmark -P 500 -r 1000 -n 1000000 eval 'return redis.call("mset","k1__rand_int__","v1__rand_int__","k2__rand_int__","v2__rand_int__","k3__rand_int__","v3__rand_int__","k4__rand_int__","v4__rand_int__xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")'
      ```
      I ran the benchmark on this branch with and without commit 68b71680a4d3bb8f0509e06578a9f15d05b92a47
      Results in requests per second:
      cmd | without optimization | without optimization 2nd run | with original optimization | with original optimization 2nd run
      -- | -- | -- | -- | --
      1 | 461233.34 | 477395.31 | 471098.16 | 469946.91
      2 | 34774.14 | 35469.8 | 35149.38 | 34464.93
      3 | 6390.59 | 6281.41 | 6146.28 | 6464.12
      4 | 28005.71 |   | 27965.77 |  
      
      As you can see, different use cases showed identical or negligible performance differences.
      So finally I decided to chuck the original optimization and simplify the code.
      9dfeda58
  33. 25 Jan, 2022 1 commit
  34. 24 Jan, 2022 1 commit
    • yoav-steinberg's avatar
      Support function flags in script EVAL via shebang header (#10126) · 7eadc5ee
      yoav-steinberg authored
      In #10025 we added a mechanism for flagging certain properties for Redis Functions.
      This lead us to think we'd like to "port" this mechanism to Redis Scripts (`EVAL`) as well. 
      
      One good reason for this, other than the added functionality is because it addresses the
      poor behavior we currently have in `EVAL` in case the script performs a (non DENY_OOM) write operation
      during OOM state. See #8478 (And a previous attempt to handle it via #10093) for details.
      Note that in Redis Functions **all** write operations (including DEL) will return an error during OOM state
      unless the function is flagged as `allow-oom` in which case no OOM checking is performed at all.
      
      This PR:
      - Enables setting `EVAL` (and `SCRIPT LOAD`) script flags as defined in #10025.
      - Provides a syntactical framework via [shebang](https://en.wikipedia.org/wiki/Shebang_(Unix)) for
        additional script annotations and even engine selection (instead of just lua) for scripts.
      - Provides backwards compatibility so scripts without the new annotations will behave as they did before.
      - Appropriate tests.
      - Changes `EVAL[SHA]/_RO` to be flagged as `STALE` commands. This makes it possible to flag individual
        scripts as `allow-stale` or not flag them as such. In backwards compatibility mode these commands will
        return the `MASTERDOWN` error as before.
      - Changes `SCRIPT LOAD` to be flagged as a `STALE` command. This is mainly to make it logically
        compatible with the change to `EVAL` in the previous point. It enables loading a script on a stale server
        which is technically okay it doesn't relate directly to the server's dataset. Running the script does, but that
        won't work unless the script is explicitly marked as `allow-stale`.
      
      Note that even though the LUA syntax doesn't support hash tag comments `.lua` files do support a shebang
      tag on the top so they can be executed on Unix systems like any shell script. LUA's `luaL_loadfile` handles
      this as part of the LUA library. In the case of `luaL_loadbuffer`, which is what Redis uses, I needed to fix the
      input script in case of a shebang manually. I did this the same way `luaL_loadfile` does, by replacing the
      first line with a single line feed character.
      7eadc5ee
  35. 14 Jan, 2022 1 commit
    • Meir Shpilraien (Spielrein)'s avatar
      Function Flags support (no-writes, no-cluster, allow-state, allow-oom) (#10066) · 4db4b434
      Meir Shpilraien (Spielrein) authored
      # Redis Functions Flags
      
      Following the discussion on #10025 Added Functions Flags support.
      The PR is divided to 2 sections:
      * Add named argument support to `redis.register_function` API.
      * Add support for function flags
      
      ## `redis.register_function` named argument support
      
      The first part of the PR adds support for named argument on `redis.register_function`, example:
      ```
      redis.register_function{
          function_name='f1',
          callback=function()
              return 'hello'
          end,
          description='some desc'
      }
      ```
      
      The positional arguments is also kept, which means that it still possible to write:
      ```
      redis.register_function('f1', function() return 'hello' end)
      ```
      
      But notice that it is no longer possible to pass the optional description argument on the positional
      argument version. Positional argument was change to allow passing only the mandatory arguments
      (function name and callback). To pass more arguments the user must use the named argument version.
      
      As with positional arguments, the `function_name` and `callback` is mandatory and an error will be
      raise if those are missing. Also, an error will be raise if an unknown argument name is given or the
      arguments type is wrong.
      
      Tests was added to verify the new syntax.
      
      ## Functions Flags
      
      The second part of the PR is adding functions flags support.
      Flags are given to Redis when the engine calls `functionLibCreateFunction`, supported flags are:
      
      * `no-writes` - indicating the function perform no writes which means that it is OK to run it on:
         * read-only replica
         * Using FCALL_RO
         * If disk error detected
         
         It will not be possible to run a function in those situations unless the function turns on the `no-writes` flag
      
      * `allow-oom` - indicate that its OK to run the function even if Redis is in OOM state, if the function will
        not turn on this flag it will not be possible to run it if OOM reached (even if the function declares `no-writes`
        and even if `fcall_ro` is used). If this flag is set, any command will be allow on OOM (even those that is
        marked with CMD_DENYOOM). The assumption is that this flag is for advance users that knows its
        meaning and understand what they are doing, and Redis trust them to not increase the memory usage.
        (e.g. it could be an INCR or a modification on an existing key, or a DEL command)
      
      * `allow-state` - indicate that its OK to run the function on stale replica, in this case we will also make
        sure the function is only perform `stale` commands and raise an error if not.
      
      * `no-cluster` - indicate to disallow running the function if cluster is enabled.
      
      Default behaviure of functions (if no flags is given):
      1. Allow functions to read and write
      2. Do not run functions on OOM
      3. Do not run functions on stale replica
      4. Allow functions on cluster
      
      ### Lua API for functions flags
      
      On Lua engine, it is possible to give functions flags as `flags` named argument:
      
      ```
      redis.register_function{function_name='f1', callback=function() return 1 end, flags={'no-writes', 'allow-oom'}, description='description'}
      ```
      
      The function flags argument must be a Lua table that contains all the requested flags, The following
      will result in an error:
      * Unknown flag
      * Wrong flag type
      
      Default behaviour is the same as if no flags are used.
      
      Tests were added to verify all flags functionality
      
      ## Additional changes
      * mark FCALL and FCALL_RO with CMD_STALE flag (unlike EVAL), so that they can run if the function was
        registered with the `allow-stale` flag.
      * Verify `CMD_STALE` on `scriptCall` (`redis.call`), so it will not be possible to call commands from script while
        stale unless the command is marked with the `CMD_STALE` flags. so that even if the function is allowed while
        stale we do not allow it to bypass the `CMD_STALE` flag of commands.
      * Flags section was added to `FUNCTION LIST` command to provide the set of flags for each function:
      ```
      > FUNCTION list withcode
      1)  1) "library_name"
          2) "test"
          3) "engine"
          4) "LUA"
          5) "description"
          6) (nil)
          7) "functions"
          8) 1) 1) "name"
                2) "f1"
                3) "description"
                4) (nil)
                5) "flags"
                6) (empty array)
          9) "library_code"
         10) "redis.register_function{function_name='f1', callback=function() return 1 end}"
      ```
      * Added API to get Redis version from within a script, The redis version can be provided using:
         1. `redis.REDIS_VERSION` - string representation of the redis version in the format of MAJOR.MINOR.PATH
         2. `redis.REDIS_VERSION_NUM` - number representation of the redis version in the format of `0x00MMmmpp`
            (`MM` - major, `mm` - minor,  `pp` - patch). The number version can be used to check if version is greater or less 
            another version. The string version can be used to return to the user or print as logs.
      
         This new API is provided to eval scripts and functions, it also possible to use this API during functions loading phase.
      4db4b434
  36. 11 Jan, 2022 1 commit
    • Binbin's avatar
      Add script tests to cover keys with expiration time set (#10096) · e22146b0
      Binbin authored
      This commit adds some tests that the test cases will
      access the keys with expiration time set in the script call.
      There was no test case for this part before. See #10080
      
      Also there is a test will cover #1525. we block the time so
      that the key can not expire in the middle of the script execution.
      
      Other changes:
      1. Delete `evalTimeSnapshot` and just use `scriptTimeSnapshot` in it's place.
      2. Some cleanups to scripting.tcl.
      3. better names for tests that run in a loop to make them distinctable 
      e22146b0