### Description ###
zfree updates memory statistics. It gets the size of the buffer from
jemalloc by calling zmalloc_size. This operation is costly. We can avoid
it if we know the buffer size. For example, we can calculate size of sds
from the data we have in its header.
This commit introduces zfree_with_size function that accepts both
pointer to a buffer, and its size. zfree is refactored to call
zfree_with_size.
sdsfree uses the new interface for all but SDS_TYPE_5.
### Benchmark ###
Dataset is 3 million strings. Each benchmark run uses its own value size
(8192, 512, and 120). The benchmark is 100% write load for 5 minutes.
```
value size new tps old tps % new us/call old us/call %
8k 272088.53 269971.75 0.78 1.83 1.92 -4.69
512 356881.91 352856.72 1.14 1.27 1.35 -5.93
120 377523.81 368774.78 2.37 1.14 1.19 -4.20
```
---------
Signed-off-by: Vadym Khoptynets <vadymkh@amazon.com>
Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
In this PR, we update master keyword to primary keyword several in
sentinel command json file and sentinelCommand function.
And there is no update for configurable parameters in sentinel.conf file
Signed-off-by: hwware <wen.hui.ware@gmail.com>
In some scenarios, the business may not be able to find the
previously used Lua script and only have a SHA signature.
Or there are multiple identical evalsha's args in monitor/slowlog,
and admin is not able to distinguish the script body.
Add a new script subcommmand to show the contents of script
given the scripts sha1. Returns a NOSCRIPT error if the script
is not present in the cache.
Usage: `SCRIPT SHOW sha1`
Complexity: `O(1)`
Closes#604.
Doc PR: https://github.com/valkey-io/valkey-doc/pull/143
---------
Signed-off-by: wei.kukey <wei.kukey@gmail.com>
Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
This PR makes our current RDB format compatible with the Redis 7.2.4 RDB
format. there are 2 changes introduced in this PR:
1. Move back the RDB version to 11
2. Make slot info section persist as AUX data instead of dedicated
section.
We have introduced slot-info as part of the work to replace cluster
metadata with slot specific dictionaries. This caused us to bump the RDB
version and thus we prevent downgrade (which is conceptualy O.K but
better be prevented). We do not require the slot-info section to exist,
so making it an AUX section will help suppport version downgrade from
Valkey 8.
fixes: [#645](https://github.com/valkey-io/valkey/issues/645)
NOTE: tested manually by:
1. connecting Redis 7.2.4 replica to a Valkey 8(RC)
2. upgrade/downgrade Redis 7.2.4 cluster and Valkey 8(RC) cluster
---------
Signed-off-by: ranshid <ranshid@amazon.com>
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
When we goto eoferr, we need to release the auxkey and auxval,
this is a cleanup, also explicitly check that decoder return
value is C_ERR.
Introduced in #586.
Signed-off-by: Binbin <binloveplay1314@qq.com>
I think the log of pfail status changes will be very useful.
The other parts were scanned and found that it can be modified.
Changes:
1. Changing pfail status releated logs from VERBOSE to NOTICE.
2. Changing configEpoch collision log from VERBOSE(warning) to NOTICE.
3. Changing some logs from DEBUG to NOTICE.
Signed-off-by: Binbin <binloveplay1314@qq.com>
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
In markNodeAsFailingIfNeeded we will count needed_quorum and failures,
needed_quorum is the half the cluster->size and plus one, and
cluster-size
is the size of primary node which contain slots, but when counting
failures, we dit not check if primary has slots.
Only the primary has slots that has the rights to vote, adding a new
clusterNodeIsVotingPrimary to formalize this concept.
Release notes:
bugfix where nodes not in the quorum group might spuriously mark nodes
as failed
---------
Signed-off-by: Binbin <binloveplay1314@qq.com>
Co-authored-by: Ping Xie <pingxie@outlook.com>
When there is a link failure while an ongoing MEET request is sent the
sending node stops sending anymore MEET and starts sending PINGs. Since
every node responds to PINGs from unknown nodes with a PONG, the
receiving node never adds the sending node. But the sending node adds
the receiving node when it sees a PONG. This can lead to asymmetry in
cluster membership. This changes makes the sender keep sending MEET
until it sees a PONG, avoiding the asymmetry.
---------
Signed-off-by: Sankar <1890648+srgsanky@users.noreply.github.com>
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>
