In replica, the key expired before master's `INCR` was arrived, so INCR
creates a new key in the replica and the test failed.
```
*** [err]: Replication of an expired key does not delete the expired key in tests/integration/replication-4.tcl
Expected '0' to be equal to '1' (context: type eval line 13 cmd {assert_equal 0 [$slave exists k]} proc ::test)
```
This test is very likely to do a false positive if the `wait_for_ofs_sync`
takes longer than the expiration time, so give it a few more chances.
The test was introduced in #9572.
Add an option "withscores" to ZRANK and ZREVRANK.
Add `[withscore]` option to both `zrank` and `zrevrank`, like this:
```
z[rev]rank key member [withscore]
```
The cluster-announce-port/cluster-announce-bus-port/cluster-announce-tls-port should take effect at runtime
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
### 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: Oran Agra <oran@redislabs.com>
Co-authored-by: Yossi Gottlieb <yossigo@gmail.com>
Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com>
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
This PR add `assert_refcount_morethan`, and modify `assert_refcount` to skip
the `OBJECT REFCOUNT` check with `needs:debug` flag. Use them to modify all
`OBJECT REFCOUNT` calls and also update the tests/README to be more specific.
The reasoning is that some of these tests could be testing something important,
and along the way also add a check for the refcount, and it could be a shame to skip
the whole test just because the refcount functionality is missing or blocked.
but much like the fact that some redis variants may not support DEBUG,
and still we want to run the majority of the test for coverage, and just skip the digest match.
This payload produces a set with duplicate elements (listpack encoding):
```
restore _key 0 "\x14\x25\x25\x00\x00\x00\x0A\x00\x06\x01\x82\x5F\x35\x03\x04\x01\x82\x5F\x31\x03\x82\x5F\x33\x03\x00\x01\x82\x5F\x39\x03\x82\x5F\x33\x03\x08\x01\x02\x01\xFF\x0B\x00\x31\xBE\x7D\x41\x01\x03\x5B\xEC"
smembers key
1) "6"
2) "_5"
3) "4"
4) "_1"
5) "_3" ---> dup
6) "0"
7) "_9"
8) "_3" ---> dup
9) "8"
10) "2"
```
This kind of sets will cause SDIFF to hang, SDIFF generated a broken
protocol and left the client hung. (Expected ten elements, but only
got nine elements due to the duplication.)
If we set `sanitize-dump-payload` to yes, we will be able to find
the duplicate elements and report "ERR Bad data format".
Discovered and discussed in #11290.
This PR also improve prints when corrupt-dump-fuzzer hangs, it will
print the cmds and the payload, an example like:
```
Testing integration/corrupt-dump-fuzzer
[TIMEOUT]: clients state report follows.
sock6 => (SPAWNED SERVER) pid:28884
Killing still running Redis server 28884
commands caused test to hang:
SDIFF __key
payload that caused test to hang: "\x14\balabala"
```
Co-authored-by: Oran Agra <oran@redislabs.com>
Apparently we used to set `loglevel debug` for tls in spawn_instance.
I.e. cluster and sentinel tests used to run with debug logging, only when tls mode was enabled.
this was probably a leftover from when creating the tls mode tests.
it cause a new test created for #11214 to fail in tls mode.
At the same time, in order to better distinguish the tests, change the
name of `test-centos7-tls` to `test-centos7-tls-module`, change the name
of `test-centos7-tls-no-tls` to `test-centos7-tls-module-no-tls`.
Note that in `test-centos7-tls-module`, we did not pass `--tls-module`
in sentinel test because it is not supported, see 4faddf1, added in #9320.
So only `test-ubuntu-tls` fails in daily CI.
Co-authored-by: Oran Agra <oran@redislabs.com>
The following example will create an empty set (listpack encoding):
```
> RESTORE key 0
"\x14\x25\x25\x00\x00\x00\x00\x00\x02\x01\x82\x5F\x37\x03\x06\x01\x82\x5F\x35\x03\x82\x5F\x33\x03\x00\x01\x82\x5F\x31\x03\x82\x5F\x39\x03\x04\xA9\x08\x01\xFF\x0B\x00\xA3\x26\x49\xB4\x86\xB0\x0F\x41"
OK
> SCARD key
(integer) 0
> SRANDMEMBER key
Error: Server closed the connection
```
In the spirit of #9297, skip empty set when loading RDB_TYPE_SET_LISTPACK.
Introduced in #11290
Introduce Shard IDs to logically group nodes in cluster mode.
1. Added a new "shard_id" field to "cluster nodes" output and nodes.conf after "hostname"
2. Added a new PING extension to propagate "shard_id"
3. Handled upgrade from pre-7.2 releases automatically
4. Refactored PING extension assembling/parsing logic
Behavior of Shard IDs:
Replicas will always follow the shards of their reported primaries. If a primary updates its shard ID, the replica will follow. (This need not follow for cluster v2) This is not an expected use case.
Improve memory efficiency of list keys
## Description of the feature
The new listpack encoding uses the old `list-max-listpack-size` config
to perform the conversion, which we can think it of as a node inside a
quicklist, but without 80 bytes overhead (internal fragmentation included)
of quicklist and quicklistNode structs.
For example, a list key with 5 items of 10 chars each, now takes 128 bytes
instead of 208 it used to take.
## Conversion rules
* Convert listpack to quicklist
When the listpack length or size reaches the `list-max-listpack-size` limit,
it will be converted to a quicklist.
* Convert quicklist to listpack
When a quicklist has only one node, and its length or size is reduced to half
of the `list-max-listpack-size` limit, it will be converted to a listpack.
This is done to avoid frequent conversions when we add or remove at the bounding size or length.
## Interface changes
1. add list entry param to listTypeSetIteratorDirection
When list encoding is listpack, `listTypeIterator->lpi` points to the next entry of current entry,
so when changing the direction, we need to use the current node (listTypeEntry->p) to
update `listTypeIterator->lpi` to the next node in the reverse direction.
## Benchmark
### Listpack VS Quicklist with one node
* LPUSH - roughly 0.3% improvement
* LRANGE - roughly 13% improvement
### Both are quicklist
* LRANGE - roughly 3% improvement
* LRANGE without pipeline - roughly 3% improvement
From the benchmark, as we can see from the results
1. When list is quicklist encoding, LRANGE improves performance by <5%.
2. When list is listpack encoding, LRANGE improves performance by ~13%,
the main enhancement is brought by `addListListpackRangeReply()`.
## Memory usage
1M lists(key:0~key:1000000) with 5 items of 10 chars ("hellohello") each.
shows memory usage down by 35.49%, from 214MB to 138MB.
## Note
1. Add conversion callback to support doing some work before conversion
Since the quicklist iterator decompresses the current node when it is released, we can
no longer decompress the quicklist after we convert the list.
Both functions and eval are marked as "no-monitor", since we want to explicitly feed in the script command before the commands generated by the script. Note that we want this behavior generally, so that commands can redact arguments before being added to the monitor.
The test introduced in #11482 fail on ARM (extra CI):
```
*** [err]: RESP2: RM_ReplyWithDouble: NaN in tests/unit/moduleapi/reply.tcl
Expected '-nan' to be equal to 'nan' (context: type eval line 3 cmd
{assert_equal "-nan" [r rw.double 0 0]} proc ::test)
*** [err]: RESP3: RM_ReplyWithDouble: NaN in tests/unit/moduleapi/reply.tcl
Expected ',-nan' to be equal to ',nan' (context: type eval line 8 cmd
{assert_equal ",-nan" [r rw.double 0 0]} proc ::test)
```
It looks like there is no negative nan on ARM.
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: Madelyn Olson <34459052+madolson@users.noreply.github.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
The test introduced in #11482 fail on mac:
```
*** [err]: RESP3: RM_ReplyWithDouble: inf in tests/unit/moduleapi/reply.tcl
Expected 'Inf' to be equal to 'inf'
(context: type eval line 6 cmd {assert_equal Inf [r rw.double inf]} proc ::test)
```
Looks like the mac platform returns inf instead of Inf in this case, this PR
uses readraw to verify the protocol.
Adding a test to cover the already existing behavior of NAN replies,
to accompany the PR that adds them to the RESP3 spec:
https://github.com/redis/redis-specifications/pull/10
This PR also covers Inf replies that are already in the spec, as well as RESP2 coverage.
Fix a few issues with the recent #11463
* use exitFromChild instead of exit
* test should ignore defunct process since that's what we expect to
happen for thees child processes when the parent dies.
* fix typo
Co-authored-by: Binbin <binloveplay1314@qq.com>
Small sets with not only integer elements are listpack encoded, by default
up to 128 elements, max 64 bytes per element, new config `set-max-listpack-entries`
and `set-max-listpack-value`. This saves memory for small sets compared to using a hashtable.
Sets with only integers, even very small sets, are still intset encoded (up to 1G
limit, etc.). Larger sets are hashtable encoded.
This PR increments the RDB version, and has an effect on OBJECT ENCODING
Possible conversions when elements are added:
intset -> listpack
listpack -> hashtable
intset -> hashtable
Note: No conversion happens when elements are deleted. If all elements are
deleted and then added again, the set is deleted and recreated, thus implicitly
converted to a smaller encoding.
During a diskless sync, if the master main process crashes, the child would
have hung in `write`. This fix closes the read fd on the child side, so that if the
parent crashes, the child will get a write error and exit.
This change also fixes disk-based replication, BGSAVE and AOFRW.
In that case the child wouldn't have been hang, it would have just kept
running until done which may be pointless.
There is a certain degree of risk here. in case there's a BGSAVE child that could
maybe succeed and the parent dies for some reason, the old code would have let
the child keep running and maybe succeed and avoid data loss.
On the other hand, if the parent is restarted, it would have loaded an old rdb file
(or none), and then the child could reach the end and rename the rdb file (data
conflicting with what the parent has), or also have a race with another BGSAVE
child that the new parent started.
Note that i removed a comment saying a write error will be ignored in the child
and handled by the parent (this comment was very old and i don't think relevant).
Introduce socket `shutdown()` into connection type, and use it
on normal socket if a fork is active. This allows us to close
client connections when there are child processes sharing the
file descriptors.
Fixes#10077. The reason is that since the `fork()` child is holding
the file descriptors, the `close` in `unlinkClient -> connClose`
isn't sufficient. The client will not realize that the connection is
disconnected until the child process ends.
Let's try to be conservative and only use shutdown when the fork is active.
Today we don't place any specific restrictions on module command names.
This can cause ambiguous scenarios. For example, someone might name a
command like "module|feature" which would be incorrectly parsed by the
ACL system as a subcommand.
In this PR, we will block some chars that we know can mess things up.
Specifically ones that can appear ok at first and cause problems in some
cases (we rather surface the issue right away).
There are these characters:
* ` ` (space) - issues with old inline protocol.
* `\r`, `\n` (newline) - can mess up the protocol on acl error replies.
* `|` - sub-commands.
* `@` - ACL categories
* `=`, `,` - info and client list fields.
note that we decided to leave `:` out as it's handled by `getSafeInfoString`
and is more likely to already been used by existing modules.
Resolve an edge case where the ID of a stream is updated retroactively
to an ID lower than the already set max_deleted_entry_id.
Currently, if we have command as below:
**xsetid mystream 1-1 MAXDELETEDID 1-2**
Then we will get the following error:
**(error) ERR The ID specified in XSETID is smaller than the provided max_deleted_entry_id**
Becuase the provided MAXDELETEDID 1-2 is greated than input last-id: 1-1
Then we could assume there is a similar situation:
step 1: we add three items in the mystream
**127.0.0.1:6381> xadd mystream 1-1 a 1
"1-1"
127.0.0.1:6381> xadd mystream 1-2 b 2
"1-2"
127.0.0.1:6381> xadd mystream 1-3 c 3
"1-3"**
step 2: we could check the mystream infomation as below:
**127.0.0.1:6381> xinfo stream mystream
1) "length"
2) (integer) 3
7) "last-generated-id"
8) "1-3"
9) "max-deleted-entry-id"
10) "0-0"
step 3: we delete the item id 1-2 and 1-3 as below:
**127.0.0.1:6381> xdel mystream 1-2
(integer) 1
127.0.0.1:6381> xdel mystream 1-3
(integer) 1**
step 4: we check the mystream information:
127.0.0.1:6381> xinfo stream mystream
1) "length"
2) (integer) 1
7) "last-generated-id"
8) "1-3"
9) "max-deleted-entry-id"
10) "1-3"
we could notice that the **max-deleted-entry-id update to 1-3**, so right now, if we just run:
**xsetid mystream 1-2**
the above command has the same effect with **xsetid mystream 1-2 MAXDELETEDID 1-3**
So we should return an error to the client that **(error) ERR The ID specified in XSETID is smaller than current max_deleted_entry_id**
According to the source code, the commands can be executed with only key name,
and no GET/SET/INCR operation arguments.
change the docs to reflect that by marking these arguments as optional.
also add tests.
Renamed from "Pause Clients" to "Pause Actions" since the mechanism can pause
several actions in redis, not just clients (e.g. eviction, expiration).
Previously each pause purpose (which has a timeout that's tracked separately from others purposes),
also implicitly dictated what it pauses (reads, writes, eviction, etc). Now it is explicit, and
the actions that are paused (bit flags) are defined separately from the purpose.
- Previously, when using feature pause-client it also implicitly means to make the server static:
- Pause replica traffic
- Pauses eviction processing
- Pauses expire processing
Making the server static is used also for failover and shutdown. This PR internally rebrand
pause-client API to become pause-action API. It also Simplifies pauseClients structure
by replacing pointers array with static array.
The context of this PR is to add another trigger to pause-client which will activated in case
of OOM as throttling mechanism ([see here](https://github.com/redis/redis/issues/10907)).
In this case we want only to pause client, and eviction actions.
RM_Call is designed to let modules call redis commands disregarding the
OOM state (the module is responsible to declare its command flags to redis,
or perform the necessary checks).
The other (new) alternative is to pass the "M" flag to RM_Call so that redis can
OOM reject commands implicitly.
However, Currently, RM_Call enforces OOM on scripts (excluding scripts that
declared `allow-oom`) in all cases, regardless of the RM_Call "M" flag being present.
This PR fixes scripts to be consistent with other commands being executed by RM_Call.
It modifies the flow in effect treats scripts as if they if they have the ALLOW_OOM script
flag, if the "M" flag is not passed (i.e. no OOM checking is being performed by RM_Call,
so no OOM checking should be done on script).
Co-authored-by: Oran Agra <oran@redislabs.com>
This is a rare failure mode of a new feature of redis 7 introduced in #9217
(when the incremental part of the ID overflows).
Till now, the outcome of that error was undetermined (could easily result in
`Elements are too large to be stored` wrongly, due to unset `errno`).
The following two cases will create an empty destkey HLL:
1. called with no source keys, like `pfmerge destkey`
2. called with non-existing source keys, like `pfmerge destkey non-existing-source-key`
In the first case, in `PFMERGE`, the dest key is actually one of the source keys too.
So `PFMERGE k1 k2` is equivalent to `SUNIONSTORE k1 k1 k2`,
and `PFMERGE k1` is equivalent to `SUNIONSTORE k1 k1`.
So the first case is reasonable, the source key is actually optional.
And the second case, `PFMERGE` on missing keys should succeed and create an empty dest.
This is consistent with `PFCOUNT`, and also with `SUNIONSTORE`, no need to change.
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: Viktor Söderqvist <viktor@zuiderkwast.se>
The use case is a module that wants to implement a blocking command on a key that
necessarily exists and wants to unblock the client in case the key is deleted (much like
what we implemented for XREADGROUP in #10306)
New module API:
* RedisModule_BlockClientOnKeysWithFlags
Flags:
* REDISMODULE_BLOCK_UNBLOCK_NONE
* REDISMODULE_BLOCK_UNBLOCK_DELETED
### Detailed description of code changes
blocked.c:
1. Both module and stream functions are called whether the key exists or not, regardless of
its type. We do that in order to allow modules/stream to unblock the client in case the key
is no longer present or has changed type (the behavior for streams didn't change, just code
that moved into serveClientsBlockedOnStreamKey)
2. Make sure afterCommand is called in serveClientsBlockedOnKeyByModule, in order to propagate
actions from moduleTryServeClientBlockedOnKey.
3. handleClientsBlockedOnKeys: call propagatePendingCommands directly after lookupKeyReadWithFlags
to prevent a possible lazy-expire DEL from being mixed with any command propagated by the
preceding functions.
4. blockForKeys: Caller can specifiy that it wants to be awakened if key is deleted.
Minor optimizations (use dictAddRaw).
5. signalKeyAsReady became signalKeyAsReadyLogic which can take a boolean in case the key is deleted.
It will only signal if there's at least one client that awaits key deletion (to save calls to
handleClientsBlockedOnKeys).
Minor optimizations (use dictAddRaw)
db.c:
1. scanDatabaseForDeletedStreams is now scanDatabaseForDeletedKeys and will signalKeyAsReady
for any key that was removed from the database or changed type. It is the responsibility of the code
in blocked.c to ignore or act on deleted/type-changed keys.
2. Use the new signalDeletedKeyAsReady where needed
blockedonkey.c + tcl:
1. Added test of new capabilities (FSL.BPOPGT now requires the key to exist in order to work)
### Background
The issue is that when saving an RDB with module AUX data, the module AUX metadata
(moduleid, when, ...) is saved to the RDB even though the module did not saved any actual data.
This prevent loading the RDB in the absence of the module (although there is no actual data in
the RDB that requires the module to be loaded).
### Solution
The solution suggested in this PR is that module AUX will be saved on the RDB only if the module
actually saved something during `aux_save` function.
To support backward compatibility, we introduce `aux_save2` callback that acts the same as
`aux_save` with the tiny change of avoid saving the aux field if no data was actually saved by
the module. Modules can use the new API to make sure that if they have no data to save,
then it will be possible to load the created RDB even without the module.
### Concerns
A module may register for the aux load and save hooks just in order to be notified when
saving or loading starts or completed (there are better ways to do that, but it still possible
that someone used it).
However, if a module didn't save a single field in the save callback, it means it's not allowed
to read in the read callback, since it has no way to distinguish between empty and non-empty
payloads. furthermore, it means that if the module did that, it must never change it, since it'll
break compatibility with it's old RDB files, so this is really not a valid use case.
Since some modules (ones who currently save one field indicating an empty payload), need
to know if saving an empty payload is valid, and if Redis is gonna ignore an empty payload
or store it, we opted to add a new API (rather than change behavior of an existing API and
expect modules to check the redis version)
### Technical Details
To avoid saving AUX data on RDB, we change the code to first save the AUX metadata
(moduleid, when, ...) into a temporary buffer. The buffer is then flushed to the rio at the first
time the module makes a write operation inside the `aux_save` function. If the module saves
nothing (and `aux_save2` was used), the entire temporary buffer is simply dropped and no
data about this AUX field is saved to the RDB. This make it possible to load the RDB even in
the absence of the module.
Test was added to verify the fix.
Motivation: for applications that use RM ACL verification functions, they would
want to return errors back to the user, in ways that are consistent with Redis.
While investigating how we should return ACL errors to the user, we realized that
Redis isn't consistent, and currently returns ACL error strings in 3 primary ways.
[For the actual implications of this change, see the "Impact" section at the bottom]
1. how it returns an error when calling a command normally
ACL_DENIED_CMD -> "this user has no permissions to run the '%s' command"
ACL_DENIED_KEY -> "this user has no permissions to access one of the keys used as arguments"
ACL_DENIED_CHANNEL -> "this user has no permissions to access one of the channels used as arguments"
2. how it returns an error when calling via 'acl dryrun' command
ACL_DENIED_CMD -> "This user has no permissions to run the '%s' command"
ACL_DENIED_KEY -> "This user has no permissions to access the '%s' key"
ACL_DENIED_CHANNEL -> "This user has no permissions to access the '%s' channel"
3. how it returns an error via RM_Call (and scripting is similar).
ACL_DENIED_CMD -> "can't run this command or subcommand";
ACL_DENIED_KEY -> "can't access at least one of the keys mentioned in the command arguments";
ACL_DENIED_CHANNEL -> "can't publish to the channel mentioned in the command";
In addition, if one wants to use RM_Call's "dry run" capability instead of the RM ACL
functions directly, one also sees a different problem than it returns ACL errors with a -ERR,
not a -PERM, so it can't be returned directly to the caller.
This PR modifies the code to generate a base message in a common manner with the ability
to set verbose flag for acl dry run errors, and keep it unset for normal/rm_call/script cases
```c
sds getAclErrorMessage(int acl_res, user *user, struct redisCommand *cmd, sds errored_val, int verbose) {
switch (acl_res) {
case ACL_DENIED_CMD:
return sdscatfmt(sdsempty(), "User %S has no permissions to run "
"the '%S' command", user->name, cmd->fullname);
case ACL_DENIED_KEY:
if (verbose) {
return sdscatfmt(sdsempty(), "User %S has no permissions to access "
"the '%S' key", user->name, errored_val);
} else {
return sdsnew("No permissions to access a key");
}
case ACL_DENIED_CHANNEL:
if (verbose) {
return sdscatfmt(sdsempty(), "User %S has no permissions to access "
"the '%S' channel", user->name, errored_val);
} else {
return sdsnew("No permissions to access a channel");
}
}
```
The caller can append/prepend the message (adding NOPERM for normal/RM_Call or indicating it's within a script).
Impact:
- Plain commands, as well as scripts and RM_Call now include the user name.
- ACL DRYRUN remains the only one that's verbose (mentions the offending channel or key name)
- Changes RM_Call ACL errors from being a `-ERR` to being `-NOPERM` (besides for textual changes)
**This somewhat a breaking change, but it only affects the RM_Call with both `C` and `E`, or `D`**
- Changes ACL errors in scripts textually from being
`The user executing the script <old non unified text>`
to
`ACL failure in script: <new unified text>`
As discussed on #11084, `propagatePendingCommands` should happened after the del
notification is fired so that the notification effect and the `del` will be replicated inside MULTI EXEC.
Test was added to verify the fix.
All commands / use cases that heavily rely on double to a string representation conversion,
(e.g. meaning take a double-precision floating-point number like 1.5 and return a string like "1.5" ),
could benefit from a performance boost by swapping snprintf(buf,len,"%.17g",value) by the
equivalent [fpconv_dtoa](https://github.com/night-shift/fpconv) or any other algorithm that ensures
100% coverage of conversion.
This is a well-studied topic and Projects like MongoDB. RedPanda, PyTorch leverage libraries
( fmtlib ) that use the optimized double to string conversion underneath.
The positive impact can be substantial. This PR uses the grisu2 approach ( grisu explained on
https://www.cs.tufts.edu/~nr/cs257/archive/florian-loitsch/printf.pdf section 5 ).
test suite changes:
Despite being compatible, in some cases it produces a different result from printf, and some tests
had to be adjusted.
one case is that `%.17g` (which means %e or %f which ever is shorter), chose to use `5000000000`
instead of 5e+9, which sounds like a bug?
In other cases, we changed TCL to compare numbers instead of strings to ignore minor rounding
issues (`expr 0.8 == 0.79999999999999999`)
When using the MIGRATE, with a destination Redis that has the user name or password set to the string "keys",
Redis would have determine the wrong set of key names the command is gonna access.
This lead to ACL returning wrong authentication result.
Destination instance:
```
127.0.0.1:6380> acl setuser default >keys
OK
127.0.0.1:6380> acl setuser keys on nopass ~* &* +@all
OK
```
Source instance:
```
127.0.0.1:6379> set a 123
OK
127.0.0.1:6379> acl setuser cc on nopass ~a* +@all
OK
127.0.0.1:6379> auth cc 1
OK
127.0.0.1:6379> migrate 127.0.0.1 6380 "" 0 1000 auth keys keys a
(error) NOPERM this user has no permissions to access one of the keys used as arguments
127.0.0.1:6379> migrate 127.0.0.1 6380 "" 0 1000 auth2 keys pswd keys a
(error) NOPERM this user has no permissions to access one of the keys used as arguments
```
Using `acl dryrun` we know that the parameters of `auth` and `auth2` are mistaken for the `keys` option.
```
127.0.0.1:6379> acl dryrun cc migrate whatever whatever "" 0 1000 auth keys keys a
"This user has no permissions to access the 'keys' key"
127.0.0.1:6379> acl dryrun cc migrate whatever whatever "" 0 1000 auth2 keys pswd keys a
"This user has no permissions to access the 'pswd' key"
```
Fix the bug by editing db.c/migrateGetKeys function, which finds the `keys` option and all the keys following.
PR #9320 introduces initialization order changes. Now cluster is initialized after modules.
This changes causes a crash if the module uses RM_Call inside the load function
on cluster mode (the code will try to access `server.cluster` which at this point is NULL).
To solve it, separate cluster initialization into 2 phases:
1. Structure initialization that happened before the modules initialization
2. Listener initialization that happened after.
Test was added to verify the fix.
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.
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.
Refine getTimeoutFromObjectOrReply() out-of-range check.
Timeout is parsed (and verifies out of range) as double and
multiplied by 1000, added mstime() and stored in long-long
which might lead to out-of-range value of long-long.
Co-authored-by: moticless <moticless@github.com>
Co-authored-by: Oran Agra <oran@redislabs.com>
Co-authored-by: Ozan Tezcan <ozantezcan@gmail.com>
This PR introduces a couple of changes to improve cluster test stability:
1. Increase the cluster node timeout to 3 seconds, which is similar to the
normal cluster tests, but introduce a new mechanism to increase the ping
period so that the tests are still fast. This new config is a debug config.
2. Set `cluster-replica-no-failover yes` on a wider array of tests which are
sensitive to failovers. This was occurring on the ARM CI.
There is a race condition in the test:
```
*** [err]: redis-cli --cluster add-node with cluster-port in tests/unit/cluster/cli.tcl
Expected '5' to be equal to '4' {assert_equal 5 [CI 0 cluster_known_nodes]} proc ::test)
```
When using cli to add node, there can potentially be a race condition
in which all nodes presenting cluster state o.k even though the added
node did not yet meet all cluster nodes.
This comment and the fix were taken from #11221. Also apply it in several
other similar places.
Mainly fix two minor bug
1. When handle BL*POP/BLMOVE commands with blocked clients, we should increment server.dirty.
2. `listPopRangeAndReplyWithKey()` in `serveClientBlockedOnList()` should not repeat calling
`signalModifiedKey()` which has been called when an element was pushed on the list.
(was skipped in all bpop commands, other than blmpop)
Other optimization
add `signal` param for `listElementsRemoved` to skip `signalModifiedKey()` to unify all pop operation.
Unifying all pop operations also prepares us for #11303, so that we can avoid having to deal with the
conversion from quicklist to listpack() in various places when the list shrinks.
