getKeysResults is typically initialized with 2kb of zeros (16 * 256),
which isn't strictly necessary since the only thing we have to
initialize is some of the metadata fields. The rest of the data can
remain junk as long as we don't access it. This was a bit of a
regression in 7.0 with the keyspecs, since we doubled the size of the
zeros, but hopefully this recovers a lot of the performance drop.
I saw a modest performance bump for deep pipeline of cluster mode (~8%).
I think we would see some comparable improvements in the other places
where we are using it such as tracking and ACLs.
---------
Signed-off-by: Madelyn Olson <matolson@amazon.com>
In ad28d222edcef9d4496fd7a94656013f07dd08e5, we added a Lua eval
scripts eviction. If the script was previously added via EVAL, we
promote it to SCRIPT LOAD, prevent it from being evicted later.
Signed-off-by: Binbin <binloveplay1314@qq.com>
This patch try to correct the latency report.
1. Rename Redis to Valkey.
2. Remove redundant Dave dialog, and refine the output message.
---------
Signed-off-by: Wenwen Chen <wenwen.chen@samsung.com>
Signed-off-by: hwware <wen.hui.ware@gmail.com>
Until now, these configuration items allowed typing empty strings,
but empty strings behave strangely.
Empty dir will fail in chdir with No such file or directory:
```
./src/valkey-server --dir ""
*** FATAL CONFIG FILE ERROR (Version 255.255.255) ***
Reading the configuration file, at line 2
>>> 'dir ""'
No such file or directory
```
Empty dbfilename will cause shutdown to fail since it will
always fail in rdb save:
```
./src/valkey-server --dbfilename ""
* User requested shutdown...
* Saving the final RDB snapshot before exiting.
# Error moving temp DB file temp-19530.rdb on the final destination (in server root dir /xxx/xxx/valkey): No such file or directory
# Error trying to save the DB, can't exit.
# Errors trying to shut down the server. Check the logs for more information.
```
Empty cluster-config-file will fail in clusterLockConfig:
```
./src/valkey-server --cluster-enabled yes --cluster-config-file ""
Can't open in order to acquire a lock: No such file or directory
```
With this patch, now we will just reject it in config set like:
```
*** FATAL CONFIG FILE ERROR (Version 255.255.255) ***
Reading the configuration file, at line 2
>>> 'xxx ""'
xxx can't be empty
```
Signed-off-by: Binbin <binloveplay1314@qq.com>
While debugging a cluster bus issue, found the cluster node flags were
represented in numbers. I generally find it easy when these are
represented as bitwise shift operation. It improves readability a bit.
Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
Added a wait_for_condition to avoid the timing issue.
```
*** [err]: query buffer resized correctly in tests/unit/querybuf.tcl
Expected 11 >= 16384 && 11 <= 32770 (context: type eval line 24 cmd {assert {$orig_test_client_qbuf >= 16384 && $orig_test_client_qbuf <= $MAX_QUERY_BUFFER_SIZE}} proc ::test)
*** [err]: query buffer resized correctly when not idle in tests/unit/querybuf.tcl
Expected 11 > 32768 (context: type eval line 14 cmd {assert {$orig_test_client_qbuf > 32768}} proc ::test)
*** [err]: query buffer resized correctly with fat argv in tests/unit/querybuf.tcl
query buffer should not be resized when client idle time smaller than 2s
```
Signed-off-by: Uri Yagelnik <uriy@amazon.com>
Allow CLUSTER subcommands NODES, INFO, MYID, MYSHARDID while loading
data.
It's safe to allow them and it's helpful for clients to get cluster
nodes/info information
during a node failover and while loading data to monitor the
state of the cluster.
---------
Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
We've been seeing some pretty consistent failures from
`test-valgrind-test` and `test-sanitizer-address` because of the
querybuf test periodically failing. I tracked it down to the test
periodically taking too long and the client cron getting triggered. A
simple solution is to just disable the cron during the key race
condition. I was able to run this locally for 100 iterations without
seeing a failure.
Example:
https://github.com/valkey-io/valkey/actions/runs/9474458354/job/26104103514
and
https://github.com/valkey-io/valkey/actions/runs/9474458354/job/26104106830.
Signed-off-by: Madelyn Olson <matolson@amazon.com>
We added some clang-format off comments before we had decided on the
format configuration. Now, it turns out that turning formatting off is
often not necessary.
---------
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Currently, "config rewrite" writes some default value in the config file
incase of empty config file specified.
But it adds multiple "save" config entries as follows:
```
save 3600 1
save 300 100
save 60 10000
```
After the fix the save will look like:
```
save 3600 1 300 100 60 10000
```
---------
Signed-off-by: Shivshankar-Reddy <shiva.sheri.github@gmail.com>
Safe iterators must call resetIterators when they are done being used.
Fix one issue where a safe iterator was not correctly calling reset
during cluster slot caching and fixed a second issue where reset
iterator was being called twice. For the double release case,
kvstoreIteratorNextDict is responsible for patching up the iterator, but
we were calling it a second time in kvstoreIteratorNext.
In addition, I added some documentation around initializing iterators,
added an assert to prevent double initialization, and remove a function
from the public interface which isn't needed and might lead to incorrect
usage of the safe iterators.
Bumping srgsanky for finding it here:
c4782066e7 (r142867004).
---------
Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Make the one backwards compatible config change we are allowed to
replace for removing master from our API.
`masterauth` and `masteruser` are still used as an alias, but aren't
explicitly referenced. As an addendum to
https://github.com/valkey-io/valkey/pull/591, it would be good to have
this in 8. Given the related PR for updated other references for master,
I just updated the ones around this specific change.
Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
More rebranding of
* Log messages (#252)
* The DENIED error reply
* Internal function names and comments, mainly Lua API
---------
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
makes SERVER_CFLAGS='-DSERVER_TEST' compile as well
Introduced in #443.
Signed-off-by: Eran Liberty <eranl@amazon.com>
Co-authored-by: Eran Liberty <eranl@amazon.com>
Remove the unused value duplicate API from dict. It's unused in the codebase and introduces unnecessary overhead.
---------
Signed-off-by: Eran Liberty <eran.liberty@gmail.com>
"Client blocked on XREADGROUP while stream's slot is migrated" uses the
migrate command, which requires special handling for TLS and non-tls.
This was not being handled, so was throwing an error.
Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Test `query buffer resized correctly` start to fail
(https://github.com/valkey-io/valkey/actions/runs/9278013807) with
non-jemalloc allocators after
https://github.com/valkey-io/valkey/pull/258 PR.
With Jemalloc we allocate ~20K for the query buffer, in the test we read
1 byte in the first read, in the second read we make sure we have at
least 16KB free place in the query buffer and we have as Jemalloc
allocated 20KB, But with non jemalloc we allocate in the first read
exactly 16KB. in the second read we check and see that we don't have
16KB free space as we already read 1 byte hence we reallocate this time
greedly (*2 of the requested size of 16KB+1) hence the test condition
that the querybuf size is < 32KB is no longer true
The `query buffer resized correctly test` starts
[failing](https://github.com/valkey-io/valkey/actions/runs/9278013807)
with non-jemalloc allocators after PR #258 .
With jemalloc, we allocate ~20KB for the query buffer. In the test, we
read 1 byte initially and then ensure there is at least 16KB of free
space in the buffer for the second read, which is satisfied by
jemalloc's 20KB allocation. However, with non-jemalloc allocators, the
first read allocates exactly 16KB. When we check again, we don't have
16KB free due to the 1 byte already read. This triggers a greedy
reallocation (doubling the requested size of 16KB+1), causing the query
buffer size to exceed the 32KB limit, thus failing the test condition.
This PR adjusted the test query buffer upper limit to be 32KB +2.
Signed-off-by: Uri Yagelnik <uriy@amazon.com>
sds type should be determined based on the size of the underlying
buffer, not the logical length of the sds. Currently we truncate the
alloc field in case buffer is larger than we can handle. It leads to a
mismatch between alloc field and the actual size of the buffer. Even
considering that alloc doesn't include header size and the null
terminator.
It also leads to a waste of memory with jemalloc. For example, let's
consider creation of sds of length 253. According to the length, the
appropriate type is SDS_TYPE_8. But we allocate `253 + sizeof(struct
sdshdr8) + 1` bytes, which sums to 257 bytes. In this case jemalloc
allocates buffer from the next size bucket. With current configuration
on Linux it's 320 bytes. So we end up with 320 bytes buffer, while we
can't address more than 255.
The same happens with other types and length close enough to the
appropriate powers of 2.
The downside of the adjustment is that with allocators that do not
allocate larger than requested chunks (like GNU allocator), we switch to
a larger type "too early". It leads to small waste of memory.
Specifically: sds of length 31 takes 35 bytes instead of 33 (2 bytes
wasted) sds of length 255 takes 261 bytes instead of 259 (2 bytes
wasted) sds of length 65,535 takes 65,545 bytes instead of 65,541 (4
bytes wasted) sds of length 4,294,967,295 takes 4,294,967,313 bytes
instead of 4,294,967,305 (8 bytes wasted)
---------
Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>
Deflake chained replicas disconnect when replica re-connect with the
same master. sync_partial_ok counter might get incremented if replica
timed out during test.
Signed-off-by: naglera <anagler123@gmail.com>
When LRU/LFU enabled, Valkey does not allow using shared objects, as
value objects may be shared among many different keys and they can't
share LRU/LFU information.
However `maxmemory-policy` is modifiable at runtime. If LRU/LFU is not
enabled at start, but then enabled when some shared objects are already
used, there could be some confusion in LRU/LFU information.
For `set` command it is OK since it is going to create a new object when
LRU/LFU enabled, but `get` command will not unshare the object and just
update LRU/LFU information.
So we may duplicate the object in this case. It is a one-time task for
each key using shared objects, unless this is the case for so many keys,
there should be no serious performance degradation.
Still, LRU will be updated anyway, no matter LRU/LFU is enabled or not,
because `OBJECT IDLETIME` needs it, unless `maxmemory-policy` is set to
LFU. So idle time of a key may still be messed up.
---------
Signed-off-by: chentianjie.ctj <chentianjie.ctj@alibaba-inc.com>
Signed-off-by: Chen Tianjie <TJ_Chen@outlook.com>
Introduce a new hidden server configuration, `enable-debug-assert`, which
allows selectively enabling or disabling, at runtime, expensive or risky
assertions used primarily for debugging and testing.
Fix#569
---------
Signed-off-by: Ping Xie <pingxie@google.com>
There are currently three block types: BLOCKED_WAIT, BLOCKED_WAITAOF,
and BLOCKED_WAIT_PREREPL, used to block clients executing `WAIT`,
`WAITAOF`, and `CLUSTER SETSLOT`, respectively. They share the same
workflow: the client is blocked until replication to the expected number
of replicas completes. However, they provide different responses
depending on the commands involved. Using distinct block types leads to
code duplication and reduced readability. This PR consolidates the three
types into a single WAIT type, differentiating them using the pending
command to ensure the appropriate response is returned.
Fix#427
---------
Signed-off-by: Ping Xie <pingxie@google.com>
In #11012, we changed the way command durations were computed to handle
the same command being executed multiple times. In #11970, we added an
assert if the duration is not properly reset, potentially indicating
that a call to report statistics was missed.
I found an edge case where this happens - easily reproduced by blocking
a client on `XGROUPREAD` and migrating the stream's slot. This causes
the engine to process the `XGROUPREAD` command twice:
1. First time, we are blocked on the stream, so we wait for unblock to
come back to it a second time. In most cases, when we come back to
process the command second time after unblock, we process the command
normally, which includes recording the duration and then resetting it.
2. After unblocking we come back to process the command, and this is
where we hit the edge case - at this point, we had already migrated the
slot to another node, so we return a `MOVED` response. But when we do
that, we don’t reset the duration field.
Fix: also reset the duration when returning a `MOVED` response. I think
this is right, because the client should redirect the command to the
right node, which in turn will calculate the execution duration.
Also wrote a test which reproduces this, it fails without the fix and
passes with it.
---------
Signed-off-by: Nitai Caro <caronita@amazon.com>
Co-authored-by: Nitai Caro <caronita@amazon.com>
In #53, we will cache the CLUSTER SLOTS response to improve the
throughput and reduct the latency.
In the code snippet below, the second cluster slots will use the
old hostname:
```
config set cluster-preferred-endpoint-type hostname
config set cluster-announce-hostname old-hostname.com
multi
cluster slots
config set cluster-announce-hostname new-hostname.com
cluster slots
exec
```
When updating the hostname, in updateAnnouncedHostname, we will set
CLUSTER_TODO_SAVE_CONFIG and we will do a clearCachedClusterSlotsResponse
in clusterSaveConfigOrDie, so harmless in most cases.
Move the clearCachedClusterSlotsResponse call to clusterDoBeforeSleep
instead of scheduling it to be called in clusterSaveConfigOrDie.
Signed-off-by: Binbin <binloveplay1314@qq.com>
ref:
- https://github.com/valkey-io/valkey/pull/118 (my pervious change)
- https://github.com/valkey-io/valkey/pull/461 (issuing that clang
format checker fails due to my change)
There was an issue that clang-format cheker failed.
I don't know why I missed it and why it didn't catch.
just running `clang-format -i bitops.c` was all.
Signed-off-by: LiiNen <kjeonghoon065@gmail.com>
_This change is the thing I suggested to redis when it was BSD, and is
not just migration - this is of course more advanced_
### Issue
There is weird difference in syntax between BITPOS and BITCOUNT:
```
BITPOS key bit [start [end [BYTE | BIT]]]
BITCOUNT key [start end [BYTE | BIT]]
```
I think this might cause confusion in terms of usability.
It was not just a syntax typo error, and really works differently.
The results below are with unstable build:
```
> get TEST:ABCD
"ABCD"
> BITPOS TEST:ABCD 1 0 -1
(integer) 1
> BITCOUNT TEST:ABCD 0 -1
(integer) 9
> BITPOS TEST:ABCD 1 0
(integer) 1
> BITCOUNT TEST:ABCD 0
(error) ERR syntax error
```
### What did I fix
simply changes logic, to accept BITCOUNT also without 'end' - 'end'
become optional, like BITPOS
```
> GET TEST:ABCD
"ABCD"
> BITPOS TEST:ABCD 1 0 -1
(integer) 1
> BITCOUNT TEST:ABCD 0 -1
(integer) 9
> BITPOS TEST:ABCD 1 0
(integer) 1
> BITCOUNT TEST:ABCD 0
(integer) 9
```
Of course, I also fixed syntax hint:
```
# ASIS
> BITCOUNT key [start end [BYTE|BIT]]
# TOBE
> BITCOUNT key [start [end [BYTE|BIT]]]
```
### Moreover ...
I hadn't noticed that there was very small dead code in these command
logic, when I wrote PR to redis.
I found it now, when write code again, so I wrote it in valkey.
``` c
/* asis unstable */
/* bitcountCommand() */
if (!strcasecmp(c->argv[4]->ptr,"bit")) isbit = 1;
// ...
if (c->argc < 4) {
if (isbit) end = (totlen<<3) + 7;
else end = totlen-1;
}
/* bitposCommand() */
if (!strcasecmp(c->argv[5]->ptr,"bit")) isbit = 1;
// ...
if (c->argc < 5) {
if (isbit) end = (totlen<<3) + 7;
else end = totlen-1;
}
```
Bit variable (actually int) "isbit" is only being set as 1, when 'BIT'
is declared.
But we were checking whether 'isbit' is true or false in this 'if'
phrase, even if isbit could never be 1, because argc is always less than
4 (or 5 in bitpos).
I think this minor fixes will make valkey command operation more
consistent.
Of course, this PR contains just changing args from "required" to
"optional", so it will never hurt previous users.
Thanks,
---------
Signed-off-by: LiiNen <kjeonghoon065@gmail.com>
Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com>
This PR optimizes client query buffer handling in Valkey by introducing
a shared query buffer that is used by default for client reads. This
reduces memory usage by ~20KB per client by avoiding allocations for
most clients using short (<16KB) complete commands. For larger or
partial commands, the client still gets its own private buffer.
The primary changes are:
* Adding a shared query buffer `shared_qb` that clients use by default
* Modifying client querybuf initialization and reset logic
* Copying any partial query from shared to private buffer before command
execution
* Freeing idle client query buffers when empty to allow reuse of shared
buffer
* Master client query buffers are kept private as their contents need to
be preserved for replication stream
In addition to the memory savings, this change shows a 3% improvement in
latency and throughput when running with 1000 active clients.
The memory reduction may also help reduce the need to evict clients when
reaching max memory limit, as the query buffer is the main memory
consumer per client.
---------
Signed-off-by: Uri Yagelnik <uriy@amazon.com>
Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
The races are between the '$rd' client and the 'r' client in the test
case.
Test case "client input output and command process statistics" in
unit/introspection.
---------
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
The replica sends its version when initiating replication, in
pipeline with other REPLCONF commands.
The primary stores it in the client struct. Other fields are made
smaller to avoid making the client struct consume more memory.
Fixes#414.
---------
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
This aligns the behaviour with established Valkey commands with a
TIMEOUT argument, such as BLPOP.
Fix#422
Signed-off-by: Ping Xie <pingxie@google.com>
- Replaces custom atomics logic with C11 default atomics logic.
- Drops "atomicvar_api" field from server info
Closes#485
---------
Signed-off-by: adetunjii <adetunjithomas1@outlook.com>
Signed-off-by: Samuel Adetunji <adetunjithomas1@outlook.com>
Co-authored-by: teej4y <samuel.adetunji@prunny.com>
Although I think this improves the readability of individual configs,
the fact there are now 1k more lines of configs makes this overall much
harder to parse. So reverting it back to the way it was before.
`,\n [ ]+` replace with `, `.
---------
Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
After READONLY, make a cluster replica behave as its primary regarding
returning ASK redirects and TRYAGAIN.
Without this patch, a client reading from a replica cannot tell if a key
doesn't exist or if it has already been migrated to another shard as
part of an ongoing slot migration. Therefore, without an ASK redirect in
this situation, offloading reads to cluster replicas wasn't reliable.
Note: The target of a redirect is always a primary. If a client wants to
continue reading from a replica after following a redirect, it needs to
figure out the replicas of that new primary using CLUSTER SHARDS or
similar.
This is related to #21 and has been made possible by the introduction of
Replication of Slot Migration States in #445.
----
Release notes:
During cluster slot migration, replicas are able to return -ASK
redirects and -TRYAGAIN.
---------
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Undeprecate cluster slots command. This command is widely used by
clients to form the cluster topology and with the recent change to
improve performance of `CLUSTER SLOTS` command via #53 as well as us
looking to further improve the usability via #517, it makes sense to
undeprecate this command.
---------
Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
