In #9373, actually need to replace `$rd $pop blist1{t} blist2{t} 1`
with `bpop_command_two_key $rd $pop blist1{t} blist2{t} 1` but forgot
to delete the latter.
This doesn't affect the test, because the later assert_error "WRONGTYPE"
is expected (and right). And if we read $rd again, it will get the
wrong result, like 'ERR unknown command 'BLMPOP_LEFT' | 'BLMPOP_RIGHT'
All of the POP commands must not decr length below 0.
So, get_fsl will delete the key if the length is 0 (unless
the caller wished to create if doesn't exist)
Other:
1. Use REDISMODULE_WRITE where needed (POP commands)
2. Use wait_for_blokced_clients in test
Unrelated:
Use quotes instead of curly braces in zset.tcl, for variable expansion
In #7875 (Redis 6.2), we changed the sds alloc to be the usable allocation
size in order to:
> reduce the need for realloc calls by making the sds implicitly take over
the internal fragmentation
This change was done most sds functions, excluding `sdsRemoveFreeSpace` and
`sdsResize`, the reason is that in some places (e.g. clientsCronResizeQueryBuffer)
we call sdsRemoveFreeSpace when we see excessive free space and want to trim it.
so if we don't trim it exactly to size, the caller may still see excessive free space and
call it again and again.
However, this resulted in some excessive calls to realloc, even when there's no need
and it's gonna be a no-op (e.g. when reducing 15 bytes allocation to 13).
It turns out that a call for realloc with jemalloc can be expensive even if it ends up
doing nothing, so this PR adds a check using `je_nallocx`, which is cheap to avoid
the call for realloc.
in addition to that this PR unifies sdsResize and sdsRemoveFreeSpace into common
code. the difference between them was that sdsResize would avoid using SDS_TYPE_5,
since it want to keep the string ready to be resized again, while sdsRemoveFreeSpace
would permit using SDS_TYPE_5 and get an optimal memory consumption.
now both methods take a `would_regrow` argument that makes it more explicit.
the only actual impact of that is that in clientsCronResizeQueryBuffer we call both sdsResize
and sdsRemoveFreeSpace for in different cases, and we now prevent the use of SDS_TYPE_5 in both.
The new test that was added to cover this concern used to pass before this PR as well,
this PR is just a performance optimization and cleanup.
Benchmark:
`redis-benchmark -c 100 -t set -d 512 -P 10 -n 100000000`
on i7-9850H with jemalloc, shows improvement from 1021k ops/sec to 1067k (average of 3 runs).
some 4.5% improvement.
Co-authored-by: Oran Agra <oran@redislabs.com>
Authenticated users issuing specially crafted SETRANGE and SORT(_RO)
commands can trigger an integer overflow, resulting with Redis attempting
to allocate impossible amounts of memory and abort with an OOM panic.
Any value in the range of [0-1) turns to 0 when being cast from double to long long. This change rounds up instead of down for values that can't be stored precisely as long doubles.
*TL;DR*
---------------------------------------
Following the discussion over the issue [#7551](https://github.com/redis/redis/issues/7551)
We decided to refactor the client blocking code to eliminate some of the code duplications
and to rebuild the infrastructure better for future key blocking cases.
*In this PR*
---------------------------------------
1. reprocess the command once a client becomes unblocked on key (instead of running
custom code for the unblocked path that's different than the one that would have run if
blocking wasn't needed)
2. eliminate some (now) irrelevant code for handling unblocking lists/zsets/streams etc...
3. modify some tests to intercept the error in cases of error on reprocess after unblock (see
details in the notes section below)
4. replace '$' on the client argv with current stream id. Since once we reprocess the stream
XREAD we need to read from the last msg and not wait for new msg in order to prevent
endless block loop.
5. Added statistics to the info "Clients" section to report the:
* `total_blocking_keys` - number of blocking keys
* `total_blocking_keys_on_nokey` - number of blocking keys which have at least 1 client
which would like
to be unblocked on when the key is deleted.
6. Avoid expiring unblocked key during unblock. Previously we used to lookup the unblocked key
which might have been expired during the lookup. Now we lookup the key using NOTOUCH and
NOEXPIRE to avoid deleting it at this point, so propagating commands in blocked.c is no longer needed.
7. deprecated command flags. We decided to remove the CMD_CALL_STATS and CMD_CALL_SLOWLOG
and make an explicit verification in the call() function in order to decide if stats update should take place.
This should simplify the logic and also mitigate existing issues: for example module calls which are
triggered as part of AOF loading might still report stats even though they are called during AOF loading.
*Behavior changes*
---------------------------------------------------
1. As this implementation prevents writing dedicated code handling unblocked streams/lists/zsets,
since we now re-process the command once the client is unblocked some errors will be reported differently.
The old implementation used to issue
``UNBLOCKED the stream key no longer exists``
in the following cases:
- The stream key has been deleted (ie. calling DEL)
- The stream and group existed but the key type was changed by overriding it (ie. with set command)
- The key not longer exists after we swapdb with a db which does not contains this key
- After swapdb when the new db has this key but with different type.
In the new implementation the reported errors will be the same as if the command was processed after effect:
**NOGROUP** - in case key no longer exists, or **WRONGTYPE** in case the key was overridden with a different type.
2. Reprocessing the command means that some checks will be reevaluated once the
client is unblocked.
For example, ACL rules might change since the command originally was executed and
will fail once the client is unblocked.
Another example is OOM condition checks which might enable the command to run and
block but fail the command reprocess once the client is unblocked.
3. One of the changes in this PR is that no command stats are being updated once the
command is blocked (all stats will be updated once the client is unblocked). This implies
that when we have many clients blocked, users will no longer be able to get that information
from the command stats. However the information can still be gathered from the client list.
**Client blocking**
---------------------------------------------------
the blocking on key will still be triggered the same way as it is done today.
in order to block the current client on list of keys, the call to
blockForKeys will still need to be made which will perform the same as it is today:
* add the client to the list of blocked clients on each key
* keep the key with a matching list node (position in the global blocking clients list for that key)
in the client private blocking key dict.
* flag the client with CLIENT_BLOCKED
* update blocking statistics
* register the client on the timeout table
**Key Unblock**
---------------------------------------------------
Unblocking a specific key will be triggered (same as today) by calling signalKeyAsReady.
the implementation in that part will stay the same as today - adding the key to the global readyList.
The reason to maintain the readyList (as apposed to iterating over all clients blocked on the specific key)
is in order to keep the signal operation as short as possible, since it is called during the command processing.
The main change is that instead of going through a dedicated code path that operates the blocked command
we will just call processPendingCommandsAndResetClient.
**ClientUnblock (keys)**
---------------------------------------------------
1. Unblocking clients on keys will be triggered after command is
processed and during the beforeSleep
8. the general schema is:
9. For each key *k* in the readyList:
```
For each client *c* which is blocked on *k*:
in case either:
1. *k* exists AND the *k* type matches the current client blocking type
OR
2. *k* exists and *c* is blocked on module command
OR
3. *k* does not exists and *c* was blocked with the flag
unblock_on_deleted_key
do:
1. remove the client from the list of clients blocked on this key
2. remove the blocking list node from the client blocking key dict
3. remove the client from the timeout list
10. queue the client on the unblocked_clients list
11. *NEW*: call processCommandAndResetClient(c);
```
*NOTE:* for module blocked clients we will still call the moduleUnblockClientByHandle
which will queue the client for processing in moduleUnblockedClients list.
**Process Unblocked clients**
---------------------------------------------------
The process of all unblocked clients is done in the beforeSleep and no change is planned
in that part.
The general schema will be:
For each client *c* in server.unblocked_clients:
* remove client from the server.unblocked_clients
* set back the client readHandler
* continue processing the pending command and input buffer.
*Some notes regarding the new implementation*
---------------------------------------------------
1. Although it was proposed, it is currently difficult to remove the
read handler from the client while it is blocked.
The reason is that a blocked client should be unblocked when it is
disconnected, or we might consume data into void.
2. While this PR mainly keep the current blocking logic as-is, there
might be some future additions to the infrastructure that we would
like to have:
- allow non-preemptive blocking of client - sometimes we can think
that a new kind of blocking can be expected to not be preempt. for
example lets imagine we hold some keys on disk and when a command
needs to process them it will block until the keys are uploaded.
in this case we will want the client to not disconnect or be
unblocked until the process is completed (remove the client read
handler, prevent client timeout, disable unblock via debug command etc...).
- allow generic blocking based on command declared keys - we might
want to add a hook before command processing to check if any of the
declared keys require the command to block. this way it would be
easier to add new kinds of key-based blocking mechanisms.
Co-authored-by: Oran Agra <oran@redislabs.com>
Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
I've seen the `BRPOPLPUSH with multiple blocked clients` test hang.
this probably happened because rd2 blocked before rd1 and then it was
also released first, and rd1 remained blocked.
```
r del blist{t} target1{t} target2{t}
r set target1{t} nolist
$rd1 brpoplpush blist{t} target1{t} 0
$rd2 brpoplpush blist{t} target2{t} 0
r lpush blist{t} foo
assert_error "WRONGTYPE*" {$rd1 read}
assert_equal {foo} [$rd2 read]
assert_equal {foo} [r lrange target2{t} 0 -1]
```
changes:
* added all missing calls for wait_for_blocked_client after issuing blocking commands)
* removed some excessive `after 100`
* fix undetected crossslot error in BRPOPLPUSH test
* rollback changes to proto-max-bulk-len so external tests can be rerun
In #11290, we added listpack encoding for SET object.
But forgot to support it in zuiFind, causes ZINTER, ZINTERSTORE,
ZINTERCARD, ZIDFF, ZDIFFSTORE to crash.
And forgot to support it in RM_ScanKey, causes it hang.
This PR add support SET listpack in zuiFind, and in RM_ScanKey.
And add tests for related commands to cover this case.
Other changes:
- There is no reason for zuiFind to go into the internals of the SET.
It can simply use setTypeIsMember and don't care about encoding.
- Remove the `#include "intset.h"` from server.h reduce the chance of
accidental intset API use.
- Move setTypeAddAux, setTypeRemoveAux and setTypeIsMemberAux
interfaces to the header.
- In scanGenericCommand, use setTypeInitIterator and setTypeNext
to handle OBJ_SET scan.
- In RM_ScanKey, improve hash scan mode, use lpGetValue like zset,
they can share code and better performance.
The zuiFind part fixes#11578
Co-authored-by: Oran Agra <oran@redislabs.com>
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
The test failed with ERR DUMP payload version or checksum are wrong.
And it only fails on CentOS, this is due to the fact that tcl8.5 does not correctly
parse the hexadecimal abbreviation. And in Ubuntu we are using tcl8.6.
1. "Fixed" the current code so that seen-time/idle actually refers to interaction
attempts (as documented; breaking change)
2. Added active-time/inactive to refer to successful interaction (what
seen-time/idle used to be)
At first, I tried to avoid changing the behavior of seen-time/idle but then realized
that, in this case, the odds are the people read the docs and implemented their
code based on the docs (which didn't match the behavior).
For the most part, that would work fine, except that issue #9996 was found.
I was working under the assumption that people relied on the docs, and for
the most part, it could have worked well enough. so instead of fixing the docs,
as I would usually do, I fixed the code to match the docs in this particular case.
Note that, in case the consumer has never read any entries, the values
for both "active-time" (XINFO FULL) and "inactive" (XINFO CONSUMERS) will
be -1, meaning here that the consumer was never active.
Note that seen/active time is only affected by XREADGROUP / X[AUTO]CLAIM, not
by XPENDING, XINFO, and other "read-only" stream CG commands (always has been,
even before this PR)
Other changes:
* Another behavioral change (arguably a bugfix) is that XREADGROUP and X[AUTO]CLAIM
create the consumer regardless of whether it was able to perform some reading/claiming
* RDB format change to save the `active_time`, and set it to the same value of `seen_time` in old rdb files.
Add an option "withscores" to ZRANK and ZREVRANK.
Add `[withscore]` option to both `zrank` and `zrevrank`, like this:
```
z[rev]rank key member [withscore]
```
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.
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.
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.
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**
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`).
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`)
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>
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.
Executing an XAUTOCLAIM command on a stream key in a specific state, with a
specially crafted COUNT argument may cause an integer overflow, a subsequent
heap overflow, and potentially lead to remote code execution.
The problem affects Redis versions 7.0.0 or newer.
This PR mainly deals with 2 crashes introduced in #9357,
and fix the QUICKLIST-PACKED-THRESHOLD mess in external test mode.
1. Fix crash due to deleting an entry from a compress quicklistNode
When inserting a large element, we need to create a new quicklistNode first,
and then delete its previous element, if the node where the deleted element is
located is compressed, it will cause a crash.
Now add `dont_compress` to quicklistNode, if we want to use a quicklistNode
after some operation, we can use this flag like following:
```c
node->dont_compress = 1; /* Prevent to be compressed */
some_operation(node); /* This operation might try to compress this node */
some_other_operation(node); /* We can use this node without decompress it */
node->dont_compress = 0; /* Re-able compression */
quicklistCompressNode(node);
```
Perhaps in the future, we could just disable the current entry from being
compressed during the iterator loop, but that would require more work.
2. Fix crash due to wrongly split quicklist
before #9357, the offset param of _quicklistSplitNode() will not negative.
For now, when offset is negative, the split extent will be wrong.
following example:
```c
int orig_start = after ? offset + 1 : 0;
int orig_extent = after ? -1 : offset;
int new_start = after ? 0 : offset;
int new_extent = after ? offset + 1 : -1;
# offset: -2, after: 1, node->count: 2
# current wrong range: [-1,-1] [0,-1]
# correct range: [1,-1] [0, 1]
```
Because only `_quicklistInsert()` splits the quicklistNode and only
`quicklistInsertAfter()`, `quicklistInsertBefore()` call _quicklistInsert(),
so `quicklistReplaceEntry()` and `listTypeInsert()` might occur this crash.
But the iterator of `listTypeInsert()` is alway from head to tail(iter->offset is
always positive), so it is not affected.
The final conclusion is this crash only occur when we insert a large element
with negative index into a list, that affects `LSET` command and `RM_ListSet`
module api.
3. In external test mode, we need to restore quicklist packed threshold after
when the end of test.
4. Show `node->count` in quicklistRepr().
5. Add new tcl proc `config_get_set` to support restoring config in tests.
Check the validity of the value before performing the create operation,
prevents new data from being generated even if the request fails to execute.
Co-authored-by: Oran Agra <oran@redislabs.com>
Co-authored-by: chendianqiang <chendianqiang@meituan.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
This PR includes 2 missed test cases of XDEL and XGROUP CREATE command
1. one test case: XDEL delete multiply id once
2. 3 test cases: XGROUP CREATE has ENTRIESREAD parameter,
which equal 0 (special positive number), 3 and negative value.
Co-authored-by: Ubuntu <lucas.guang.yang1@huawei.com>
Co-authored-by: Oran Agra <oran@redislabs.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
The temporary array for deleted entries reply of XAUTOCLAIM was
insufficient, but also in fact the COUNT argument should be used to
control the size of the reply, so instead of terminating the loop by
only counting the claimed entries, we'll count deleted entries as well.
Fix#10968
Addresses CVE-2022-31144
when we know the size of the zset we're gonna store in advance,
we can check if it's greater than the listpack encoding threshold,
in which case we can create a skiplist from the get go, and avoid
converting the listpack to skiplist later after it was already populated.
When `zrangestore` is called container destination object is created.
Before this PR we used to create a listpack based object even if `zset-max-ziplist-entries`
or equivalent`zset-max-listpack-entries` was set to 0.
This triggered immediate conversion of the listpack into a skiplist in `zrangestore`, which hits
an assertion resulting in an engine crash.
Added a TCL test that reproduces this issue.
This bug was introduced in #9484 (7.0.0).
It result that BZMPOP blocked on non-key arguments.
Like `bzmpop 0 1 myzset min count 10`, this command will additionally
block in these keys (except for the first and the last argument) and can return their values:
- 0: timeout value
- 1: numkeys value
- min: min/max token
- count: count token
Updated the comments for:
info command
lmpopCommand and blmpopCommand
sinterGenericCommand
Fix the missing "key" words in the srandmemberCommand function
For LPOS command, when rank is 0, prompt user that rank could be
positive number or negative number, and add a test for it
When user uses the same input key for SDIFF as the first one, the result must be empty, so we don't need to process the elements to test.
This method is like the one done in zset‘s `zsetChooseDiffAlgorithm`
Co-authored-by: Oran Agra <oran@redislabs.com>
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
```
Fixed a bug that used the `hincrbyfloat` or `hincrby` commands to make the field or value exceed the
`hash_max_listpack_value` but did not change the object encoding of the hash structure.
Add a length check for field and value, check the length of value first, if the length of value does not
exceed `hash_max_listpack_value` then check the length of field.
If the length of field or value is too long, it will reduce the efficiency of listpack, and the object encoding
will become hashtable after AOF restart, so this is also to keep the same before and after AOF restart.
There are a few places that use a hard coded const of 128 to allocate a buffer for d2string.
Replace these with a clear macro.
Note that In theory, converting double into string could take as much as nearly 400 chars,
but since d2string uses `%g` and not `%f`, it won't pass some 40 chars.
unrelated:
restore some changes to auto generated commands.c that got accidentally reverted in #10293
Deleting a stream while a client is blocked XREADGROUP should unblock the client.
The idea is that if a client is blocked via XREADGROUP is different from
any other blocking type in the sense that it depends on the existence of both
the key and the group. Even if the key is deleted and then revived with XADD
it won't help any clients blocked on XREADGROUP because the group no longer
exist, so they would fail with -NOGROUP anyway.
The conclusion is that it's better to unblock these clients (with error) upon
the deletion of the key, rather than waiting for the first XADD.
Other changes:
1. Slightly optimize all `serveClientsBlockedOn*` functions by checking `server.blocked_clients_by_type`
2. All `serveClientsBlockedOn*` functions now use a list iterator rather than looking at `listFirst`, relying
on `unblockClient` to delete the head of the list. Before this commit, only `serveClientsBlockedOnStreams`
used to work like that.
3. bugfix: CLIENT UNBLOCK ERROR should work even if the command doesn't have a timeout_callback
(only relevant to module commands)
Adds the ability to track the lag of a consumer group (CG), that is, the number
of entries yet-to-be-delivered from the stream.
The proposed constant-time solution is in the spirit of "best-effort."
Partially addresses #8737.
## Description of approach
We add a new "entries_added" property to the stream. This starts at 0 for a new
stream and is incremented by 1 with every `XADD`. It is essentially an all-time
counter of the entries added to the stream.
Given the stream's length and this counter value, we can trivially find the logical
"entries_added" counter of the first ID if and only if the stream is contiguous.
A fragmented stream contains one or more tombstones generated by `XDEL`s.
The new "xdel_max_id" stream property tracks the latest tombstone.
The CG also tracks its last delivered ID's as an "entries_read" counter and
increments it independently when delivering new messages, unless the this
read counter is invalid (-1 means invalid offset). When the CG's counter is
available, the reported lag is the difference between added and read counters.
Lastly, this also adds a "first_id" field to the stream structure in order to make
looking it up cheaper in most cases.
## Limitations
There are two cases in which the mechanism isn't able to track the lag.
In these cases, `XINFO` replies with `null` in the "lag" field.
The first case is when a CG is created with an arbitrary last delivered ID,
that isn't "0-0", nor the first or the last entries of the stream. In this case,
it is impossible to obtain a valid read counter (short of an O(N) operation).
The second case is when there are one or more tombstones fragmenting
the stream's entries range.
In both cases, given enough time and assuming that the consumers are
active (reading and lacking) and advancing, the CG should be able to
catch up with the tip of the stream and report zero lag.
Once that's achieved, lag tracking would resume as normal (until the
next tombstone is set).
## API changes
* `XGROUP CREATE` added with the optional named argument `[ENTRIESREAD entries-read]`
for explicitly specifying the new CG's counter.
* `XGROUP SETID` added with an optional positional argument `[ENTRIESREAD entries-read]`
for specifying the CG's counter.
* `XINFO` reports the maximal tombstone ID, the recorded first entry ID, and total
number of entries added to the stream.
* `XINFO` reports the current lag and logical read counter of CGs.
* `XSETID` is an internal command that's used in replication/aof. It has been added with
the optional positional arguments `[ENTRIESADDED entries-added] [MAXDELETEDID max-deleted-entry-id]`
for propagating the CG's offset and maximal tombstone ID of the stream.
## The generic unsolved problem
The current stream implementation doesn't provide an efficient way to obtain the
approximate/exact size of a range of entries. While it could've been nice to have
that ability (#5813) in general, let alone specifically in the context of CGs, the risk
and complexities involved in such implementation are in all likelihood prohibitive.
## A refactoring note
The `streamGetEdgeID` has been refactored to accommodate both the existing seek
of any entry as well as seeking non-deleted entries (the addition of the `skip_tombstones`
argument). Furthermore, this refactoring also migrated the seek logic to use the
`streamIterator` (rather than `raxIterator`) that was, in turn, extended with the
`skip_tombstones` Boolean struct field to control the emission of these.
Co-authored-by: Guy Benoish <guy.benoish@redislabs.com>
Co-authored-by: Oran Agra <oran@redislabs.com>
This is a followup work for #10278, and a discussion about #10279
The changes:
- fix failed_calls in command stats for blocked clients that got error.
including CLIENT UNBLOCK, and module replying an error from a thread.
- fix latency stats for XREADGROUP that filed with -NOGROUP
Theory behind which errors should be counted:
- error stats represents errors returned to the user, so an error handled by a
module should not be counted.
- total error counter should be the same.
- command stats represents execution of commands (even with RM_Call, and if
they fail or get rejected it counts these calls in commandstats, so it should
also count failed_calls)
Some thoughts about Scripts:
for scripts it could be different since they're part of user code, not the infra (not an extension to redis)
we certainly want commandstats to contain all calls and errors
a simple script is like mult-exec transaction so an error inside it should be counted in error stats
a script that replies with an error to the user (using redis.error_reply) should also be counted in error stats
but then the problem is that a plain `return redis.call("SET")` should not be counted twice (once for the SET
and once for EVAL)
so that's something left to be resolved in #10279
Fix#7021#8924#10198
# Intro
Before this commit X[AUTO]CLAIM used to transfer deleted entries from one
PEL to another, but reply with "nil" for every such entry (instead of the entry id).
The idea (for XCLAIM) was that the caller could see this "nil", realize the entry
no longer exists, and XACK it in order to remove it from PEL.
The main problem with that approach is that it assumes there's a correlation
between the index of the "id" arguments and the array indices, which there
isn't (in case some of the input IDs to XCLAIM never existed/read):
```
127.0.0.1:6379> XADD x 1 f1 v1
"1-0"
127.0.0.1:6379> XADD x 2 f1 v1
"2-0"
127.0.0.1:6379> XADD x 3 f1 v1
"3-0"
127.0.0.1:6379> XGROUP CREATE x grp 0
OK
127.0.0.1:6379> XREADGROUP GROUP grp Alice COUNT 2 STREAMS x >
1) 1) "x"
2) 1) 1) "1-0"
2) 1) "f1"
2) "v1"
2) 1) "2-0"
2) 1) "f1"
2) "v1"
127.0.0.1:6379> XDEL x 1 2
(integer) 2
127.0.0.1:6379> XCLAIM x grp Bob 0 0-99 1-0 1-99 2-0
1) (nil)
2) (nil)
```
# Changes
Now, X[AUTO]CLAIM acts in the following way:
1. If one tries to claim a deleted entry, we delete it from the PEL we found it in
(and the group PEL too). So de facto, such entry is not claimed, just cleared
from PEL (since anyway it doesn't exist in the stream)
2. since we never claim deleted entries, X[AUTO]CLAIM will never return "nil"
instead of an entry.
3. add a new element to XAUTOCLAIM's response (see below)
# Knowing which entries were cleared from the PEL
The caller may want to log any entries that were found in a PEL but deleted from
the stream itself (it would suggest that there might be a bug in the application:
trimming the stream while some entries were still no processed by the consumers)
## XCLAIM
the set {XCLAIM input ids} - {XCLAIM returned ids} contains all the entry ids that were
not claimed which means they were deleted (assuming the input contains only entries
from some PEL). The user doesn't need to XACK them because XCLAIM had already
deleted them from the source PEL.
## XAUTOCLAIM
XAUTOCLAIM has a new element added to its reply: it's an array of all the deleted
stream IDs it stumbled upon.
This is somewhat of a breaking change since X[AUTO]CLAIM used to be able to reply
with "nil" and now it can't... But since it was undocumented (and generally a bad idea
to rely on it, as explained above) the breakage is not that bad.
Summary of changes:
1. Rename `redisCommand->name` to `redisCommand->declared_name`, it is a
const char * for native commands and SDS for module commands.
2. Store the [sub]command fullname in `redisCommand->fullname` (sds).
3. List subcommands in `ACL CAT`
4. List subcommands in `COMMAND LIST`
5. `moduleUnregisterCommands` now will also free the module subcommands.
6. RM_GetCurrentCommandName returns full command name
Other changes:
1. Add `addReplyErrorArity` and `addReplyErrorExpireTime`
2. Remove `getFullCommandName` function that now is useless.
3. Some cleanups about `fullname` since now it is SDS.
4. Delete `populateSingleCommand` function from server.h that is useless.
5. Added tests to cover this change.
6. Add some module unload tests and fix the leaks
7. Make error messages uniform, make sure they always contain the full command
name and that it's quoted.
7. Fixes some typos
see the history in #9504, fixes#10124
Co-authored-by: Oran Agra <oran@redislabs.com>
Co-authored-by: guybe7 <guy.benoish@redislabs.com>
### Describe
Fix crash found by CI, Introduced by #9849.
When we do any operation on the quicklist, we should make sure that all nodes
of the quicklist should not be in the recompressed state.
### Issues
This PR fixes two issues with incorrect recompression.
1. The current quicklist node is full and the previous node isn't full,
the current node is not recompressed correctly after inserting elements into the previous node.
2. The current quicklist node is full and the next node isn't full,
the current node is not recompressed correctly after inserting elements into the next node.
### Test
Add two tests to cover incorrect compression issues.
### Other
Fix unittest test failure caused by assertion introduced by #9849.
Syntax:
`COMMAND DOCS [<command name> ...]`
Background:
Apparently old version of hiredis (and thus also redis-cli) can't
support more than 7 levels of multi-bulk nesting.
The solution is to move all the doc related metadata from COMMAND to a
new COMMAND DOCS sub-command.
The new DOCS sub-command returns a map of commands (not an array like in COMMAND),
And the same goes for the `subcommands` field inside it (also contains a map)
Besides that, the remaining new fields of COMMAND (hints, key-specs, and
sub-commands), are placed in the outer array rather than a nested map.
this was done mainly for consistency with the old format.
Other changes:
---
* Allow COMMAND INFO with no arguments, which returns all commands, so that we can some day deprecated
the plain COMMAND (no args)
* Reduce the amount of deferred replies from both COMMAND and COMMAND
DOCS, especially in the inner loops, since these create many small
reply objects, which lead to many small write syscalls and many small
TCP packets.
To make this easier, when populating the command table, we count the
history, args, and hints so we later know their size in advance.
Additionally, the movablekeys flag was moved into the flags register.
* Update generate-commands-json.py to take the data from both command, it
now executes redis-cli directly, instead of taking input from stdin.
* Sub-commands in both COMMAND (and COMMAND INFO), and also COMMAND DOCS,
show their full name. i.e. CONFIG
* GET will be shown as `config|get` rather than just `get`.
This will be visible both when asking for `COMMAND INFO config` and COMMAND INFO config|get`, but is
especially important for the later.
i.e. imagine someone doing `COMMAND INFO slowlog|get config|get` not being able to distinguish between the two
items in the array response.
It used to return `$-1` in RESP2, now we will return `*-1`.
This is a bug in redis 6.2 when COUNT was added, the `COUNT`
option was introduced in #8179. Fix#10089.
the documentation of [LPOP](https://redis.io/commands/lpop) says
```
When called without the count argument:
Bulk string reply: the value of the first element, or nil when key does not exist.
When called with the count argument:
Array reply: list of popped elements, or nil when key does not exist.
```
Fix#9410
Crucial for the ms and sequence deltas, but I changed all
calls, just in case (e.g. "flags")
Before this commit:
`ms_delta` and `seq_delta` could have overflown, causing `currid` to be wrong,
which in turn would cause `streamTrim` to trim the entire rax node (see new test)
It turns out that libc malloc can return an allocation of a different size on requests of the same size.
this means that matching MEMORY USAGE of one key to another copy of the same data can fail.
Solution:
Keep running the test that calls MEMORY USAGE, but ignore the response.
We do that by introducing a new utility function to get the memory usage, which always returns 1
when the allocator is not jemalloc.
Other changes:
Some formatting for datatype2.tcl
Co-authored-by: Oran Agra <oran@redislabs.com>
