fix typo ` the largest possible limit is 16k` -> ` the largest possible limit is 64k`.
The count field is 16 bits so the largest possible limit is 64k(2**16).
There is a timing issue reported in test-sanitizer-address (gcc):
```
Sentinels (re)connection following SENTINEL SET mymaster auth-pass:
FAILED: Expected to be disconnected from master due to wrong password
```
The reason we reach it, is because the test is fast enough to modify
auth-pass and test sentinel connection status with the server,
before its scheduled operation got the chance to update connection
status with the server.
We need to wait for `sentinelTimer` to kick in, and then update the
connection status. Replace condition with wait_for_condition on the check.
Fix just like #10480 did
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.
There are some commands that has the wrong key specs.
This PR adds a key-spec related check in generate-command-code.py.
Check if the index is valid, or if there is an unused index.
The check result will look like:
```
[root]# python utils/generate-command-code.py
Processing json files...
Linking container command to subcommands...
Checking all commands...
command: RESTORE_ASKING may have unused key_spec
command: RENAME may have unused key_spec
command: PFDEBUG may have unused key_spec
command: WATCH key_specs missing flags
command: LCS arg: key2 key_spec_index error
command: RENAMENX may have unused key_spec
Error: There are errors in the commands check, please check the above logs.
```
The following commands have been fixed according to the check results:
- RESTORE ASKING: add missing arguments section (and history section)
- RENAME: newkey's key_spec_index should be 1
- PFDEBUG: add missing arguments (and change the arity from -3 to 3)
- WATCH: add missing key_specs flags: RO, like EXIST (it allow you to know the key exists, or is modified, but doesn't "leak" the data)
- LCS: key2 key_spec_index error, there is only one key-spec
- RENAMENX: newkey's key_spec_index should be 1
instead of printing a log when a folder or a manifest is missing (level reduced), we print:
total time it took to load all the aof files
when creating a new base or incr file
starting to write to an existing incr file on startup
Added the function name/script sha to the script timeout log message.
This info existed in the log in redis 6.2, was removed in the function refactoring
since was initially complicated, but later made simple.
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
This PR does 2 main things:
1) Add warning for suspected slow system clocksource setting. This is Linux specific.
2) Add a `--check-system` argument to redis which runs all system checks and prints a report.
## System checks
Add a command line option `--check-system` which runs all known system checks and provides
a report to stdout of which systems checks have failed with details on how to reconfigure the
system for optimized redis performance.
The `--system-check` mode exists with an appropriate error code after running all the checks.
## Slow clocksource details
We check the system's clocksource performance by running `clock_gettime()` in a loop and then
checking how much time was spent in a system call (via `getrusage()`). If we spend more than
10% of the time in the kernel then we print a warning. I verified that using the slow clock sources:
`acpi_pm` (~90% in the kernel on my laptop) and `xen` (~30% in the kernel on an ec2 `m4.large`)
we get this warning.
The check runs 5 system ticks so we can detect time spent in kernel at 20% jumps (0%,20%,40%...).
Anything more accurate will require the test to run longer. Typically 5 ticks are 50ms. This means
running the test on startup will delay startup by 50ms. To avoid this we make sure the test is only
executed in the `--check-system` mode.
For a quick startup check, we specifically warn if the we see the system is using the `xen` clocksource
which we know has bad performance and isn't recommended (at least on ec2). In such a case the
user should manually rung redis with `--check-system` to force the slower clocksource test described
above.
## Other changes in the PR
* All the system checks are now implemented as functions in _syscheck.c_.
They are implemented using a standard interface (see details in _syscheck.c_).
To do this I moved the checking functions `linuxOvercommitMemoryValue()`,
`THPIsEnabled()`, `linuxMadvFreeForkBugCheck()` out of _server.c_ and _latency.c_
and into the new _syscheck.c_. When moving these functions I made sure they don't
depend on other functionality provided in _server.c_ and made them use a standard
"check functions" interface. Specifically:
* I removed all logging out of `linuxMadvFreeForkBugCheck()`. In case there's some
unexpected error during the check aborts as before, but without any logging.
It returns an error code 0 meaning the check didn't not complete.
* All these functions now return 1 on success, -1 on failure, 0 in case the check itself
cannot be completed.
* The `linuxMadvFreeForkBugCheck()` function now internally calls `exit()` and not
`exitFromChild()` because the latter is only available in _server.c_ and I wanted to
remove that dependency. This isn't an because we don't need to worry about the
child process created by the test doing anything related to the rdb/aof files which
is why `exitFromChild()` was created.
* This also fixes parsing of other /proc/\<pid\>/stat fields to correctly handle spaces
in the process name and be more robust in general. Not that before this fix the rss
info in `INFO memory` was corrupt in case of spaces in the process name. To
recreate just rename `redis-server` to `redis server`, start it, and run `INFO memory`.
Scripts that have the `no-writes` flag, cannot execute write commands,
and since all `deny-oom` commands are write commands, we now act
as if the `allow-oom` flag is implicitly set for scripts that set the `no-writes` flag.
this also implicitly means that the EVAL*_RO and FCALL_RO commands can
never fails with OOM error.
Note about a bug that's no longer relevant:
There was an issue with EVAL*_RO using shebang not being blocked correctly
in OOM state:
When an EVAL script declares a shebang, it was by default not allowed to run in
OOM state.
but this depends on a flag that is updated before the command is executed, which
was not updated in case of the `_RO` variants.
the result is that if the previous cached state was outdated (either true or false),
the script will either unjustly fail with OOM, or unjustly allowed to run despite
the OOM state.
It doesn't affect scripts without a shebang since these depend on the actual
commands they run, and since these are only read commands, they don't care
for that cached oom state flag.
it did affect scripts with shebang and no allow-oom flag, bug after the change in
this PR, scripts that are run with eval_ro would implicitly have that flag so again
the cached state doesn't matter.
p.s. this isn't a breaking change since all it does is allow scripts to run when they
should / could rather than blocking them.
Remove some dead code in object.c, ziplist is no longer used in 7.0
Some backgrounds:
zipmap - hash: replaced by ziplist in #285
ziplist - hash: replaced by listpack in #8887
ziplist - zset: replaced by listpack in #9366
ziplist - list: replaced by quicklist (listpack) in #2143 / #9740
Moved the location of ziplist.h in the server.c
Finish current loop and display the scanned keys summery on SIGINT (Ctrl-C) signal.
It will also prepend the current scanned percentage to the scanned keys summery 1st line.
In this commit I've renamed and relocated `intrinsicLatencyModeStop` function as I'm using the exact same logic.
We observed from our replication testing that when the master becomes unresponsive,
or the replication connection is broken during PSYNC so the replica doesn't get a
response from the master, it was not able to recognize that condition as a failure
and instead moved into the full-sync code path. This fix makes the replica fail and
retry the PSYNC with the master in such scenarios.
sometimes it is using `scriptIsRunning()` and other times it is using `server.in_script`.
We should use the `scriptIsRunning()` method consistently throughout the code base.
Removed server.in_script sine it's no longer used / needed.
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
The purpose of the test is to kill the child while it is running.
From the last two lines we can see the child exits before being killed.
```
- Module fork started pid: 56998
* <fork> fork child started
- Killing running module fork child: 56998
* <fork> fork child exiting
signal-handler (1652267501) Received SIGUSR1 in child, exiting now.
```
In this commit, we pass an argument to `fork.create` indicating how
long it should sleep. For the fork kill test, we use a longer time to
avoid the child exiting before being killed.
Other changes:
use wait_for_condition instead of hardcoded `after 250`.
Unify the test for failing fork with the one for killing it (save time)
This fixes a possible regression in Redis 7.0.0, in which doing CONFIG SET
on a TLS config would not reload the configuration in case the new config is
the same file as before.
A volatile configuration is a configuration value which is a reference to the
configuration data and not the configuration data itself. In such a case Redis
doesn't know if the config data changed under the hood and can't assume a
change happens only when the config value changes. Therefore it needs to
be applied even when setting a config value to the same value as it was before.
Before this commit, all source files including those that are not going
to be compiled were used. Some of these files are platform specific and
won't even pre-process on another platform. With GCC/Clang, that's not
an issue and they'll simply ignore them, but ICC aborts in this case.
This commit only attempts to generate Makefile.dep from the actual set
of C source files that will be compiled.
## Take one bulk string with spaces for MULTI_ARG configs parsing
Currently redis-server looks for arguments that start with `--`,
and anything in between them is considered arguments for the config.
like: `src/redis-server --shutdown-on-sigint nosave force now --port 6380`
MULTI_ARG configs behave differently for CONFIG command, vs the command
line argument for redis-server.
i.e. CONFIG command takes one bulk string with spaces in it, while the
command line takes an argv array with multiple values.
In this PR, in config.c, if `argc > 1` we can take them as is,
and if the config is a `MULTI_ARG` and `argc == 1`, we will split it by spaces.
So both of these will be the same:
```
redis-server --shutdown-on-sigint nosave force now --shutdown-on-sigterm nosave force
redis-server --shutdown-on-sigint nosave "force now" --shutdown-on-sigterm nosave force
redis-server --shutdown-on-sigint nosave "force now" --shutdown-on-sigterm "nosave force"
```
## Allow options value to use the `--` prefix
Currently it decides to switch to the next config, as soon as it sees `--`,
even if there was not a single value provided yet to the last config,
this makes it impossible to define a config value that has `--` prefix in it.
For instance, if we want to set the logfile to `--my--log--file`,
like `redis-server --logfile --my--log--file --loglevel verbose`,
current code will handle that incorrectly.
In this PR, now we allow a config value that has `--` prefix in it.
**But note that** something like `redis-server --some-config --config-value1 --config-value2 --loglevel debug`
would not work, because if you want to pass a value to a config starting with `--`, it can only be a single value.
like: `redis-server --some-config "--config-value1 --config-value2" --loglevel debug`
An example (using `--` prefix config value):
```
redis-server --logfile --my--log--file --loglevel verbose
redis-cli config get logfile loglevel
1) "loglevel"
2) "verbose"
3) "logfile"
4) "--my--log--file"
```
### Potentially breaking change
`redis-server --save --loglevel verbose` used to work the same as `redis-server --save "" --loglevel verbose`
now, it'll error!
## FLUSHALL
We used to restore the dirty counter after `rdbSave` zeroed it if we enable save.
Otherwise FLUSHALL will not be replicated nor put into the AOF.
And then we do increment it again below.
Without that extra dirty++, when db was already empty, FLUSHALL
will not be replicated nor put into the AOF.
We now gonna replace all that dirty counter magic with a call
to forceCommandPropagation (REPL and AOF), instead of all the
messing around with the dirty counter.
Added tests to cover three part (dirty counter, REPL, AOF).
One benefit other than cleaner code is that the `rdb_changes_since_last_save` is correct in this case.
## FLUSHDB
FLUSHDB was not replicated nor put into the AOF when db was already empty.
Unlike DEL on a non-existing key, FLUSHDB always does something, and that's to call the module hook.
So basically FLUSHDB is never a NOP, and thus it should always be propagated.
Not doing that, could mean that if a module does something in that hook, and wants to
avoid issues of that hook being missing on the replica if the db is empty, it'll need to do complicated things.
So now FLUSHDB add call forceCommandPropagation, we will always propagate FLUSHDB.
Always propagating FLUSHDB seems like a safe approach that shouldn't have any drawbacks (other than looking odd)
This was mentioned in #8972
## Test section:
We actually found it while solving a race condition in the BGSAVE test (other.tcl).
It was found in extra_ci Daily Arm64 (test-libc-malloc).
```
[exception]: Executing test client: ERR Background save already in progress.
ERR Background save already in progress
```
It look like `r flushdb` trigger (schedule) a bgsave right after `waitForBgsave r` and before `r save`.
Changing flushdb to flushall, FLUSHALL will do a foreground save and then set the dirty counter to 0.
Fix#10552
We no longer piggyback getkeys_proc to hold the RedisModuleCommand struct, when exists
Others:
Use `doesCommandHaveKeys` in `RM_GetCommandKeysWithFlags` and `getKeysSubcommandImpl`.
It causes a very minor behavioral change in commands that don't have actual keys, but have a spec
with `CMD_KEY_NOT_KEY`.
For example, before this command `COMMAND GETKEYS SPUBLISH` would return
`Invalid arguments specified for command` but not it returns `The command has no key arguments`
I suggest to use "[fpclassify](https://en.cppreference.com/w/cpp/numeric/math/fpclassify)" for float
comparison with zero, because of expression "value == 0" with value very close to zero can be
considered as true with some performance compiler optimizations.
Note: this code was introduced by 9d520a7f to accept zset scores that get ERANGE in conversion
due to precision loss near 0.
But with Intel compilers, ICC and ICX, where optimizations for 0 check are more aggressive, "==0" is
true for mentioned functions, however should not be. Behavior is seen starting from O2.
This leads to a failure in the ZSCAN test in scan.tcl
It used to returns slots as strings, like:
```
redis> cluster shards
1) 1) "slots"
2) 1) "10923"
2) "16383"
```
CLUSTER SHARDS docs and the top comment of #10293 says that it returns integers.
Note other commands like CLUSTER SLOTS, it returns slots as integers.
Use addReplyLongLong instead of addReplyBulkLongLong, now it returns slots as integers:
```
redis> cluster shards
1) 1) "slots"
2) 1) (integer) 10923
2) (integer) 16383
```
This is a small breaking change, introduced in 7.0.0 (7.0 RC3, #10293)
Fixes#10680
Set `old_li` to NULL to avoid linking it again on error.
Before the fix, loading an already existing library will cause the existing library to be added again. This cause not harm other then wrong statistics. The statistics that are effected by the issue are:
* `libraries_count` and `functions_count` returned by `function stats` command
* `used_memory_functions` returned on `info memory` command
* `functions.caches` returned on `memory stats` command
fix some typo in "t_zset.c".
1. `zzlisinlexrange` the function name mentioned in the comment is misspelled.
2. fix typo in function name`zarndmemberReplyWithListpack` -> `zrandmemberReplyWithListpack`
Unintentional change in #9644 (since RC1) meant that an empty `--save ""` config
from command line, wouldn't have clear any setting from the config file
Added tests to cover that, and improved test infra to take additional
command line args for redis-server
If we want to support bits that can be overlapping, we need to make sure
that:
1. we don't use the same bit for two return values.
2. values should be sorted so that prefer ones (matching more
bits) come first.
* Module API doc script: Mark unreleased API functions
* fix broken quotes in generate-module-api-doc.rb
Co-authored-by: Oran Agra <oran@redislabs.com>
In general, our error handler make sure the error
object is always a table. In some rare cases (such
as OOM error), the error handler will not be called
and the error object will be a string. The PR expose
the error even if its a string and not a table.
Currently there is no way to test it but if it'll ever happen,
it is better to propagate this string upwards than just
generate a generic error without any specific info.
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>
Till now, on MacOS we only used to enable SO_KEEPALIVE,
but we didn't set the interval which is configurable via the `tcp-keepalive` config.
This adds support for that on MacOS, to match what we already do on Linux.
add a comment to `container` in `quicklist.h`.
Because `PLAIN` and `PACKED` are not as easy to understand as `NONE`
and `LISTPACK` and we don't have a detailed comment on it.
Co-authored-by: Oran Agra <oran@redislabs.com>
