lookupKeyReadOrReply and lookupKeyWriteOrReply might decide to reply to the user
with the given robj reply. This reply might be an error reply and if so addReply
function is used instead of addReplyErrorObject which will cause the error reply not
to be counted on stats. The fix checks the first char in the reply and if its '-' (error)
it uses addReplyErrorObject.
REDISMODULE_CTX_FLAGS_EVICT and REDISMODULE_CTX_FLAGS_MAXMEMORY
shouldn't be set when the module is run inside a replica that doesn't do eviction.
one may argue that the database is under eviction (the master does eviction and sends DELs to the replica).
but on the other hand, we don't really know the master's configuration.
all that matters is if the current instance does evictions or not.
listenToPort attempts to gracefully handle and ignore certain errors but does not store errno prior to logging, which in turn calls several libc functions that may overwrite errno.
This has been discovered due to libmusl strftime() always returning with errno set to EINVAL, which resulted with docker-library/redis#273.
1. moduleReplicateMultiIfNeeded should use server.in_eval like
moduleHandlePropagationAfterCommandCallback
2. server.in_eval could have been set to 1 and not reset back
to 0 (a lot of missed early-exits after in_eval is already 1)
Note: The new assertions in processCommand cover (2) and I added
two module tests to cover (1)
Implications:
If an EVAL that failed (and thus left server.in_eval=1) runs before a module
command that replicates, the replication stream will contain MULTI (because
moduleReplicateMultiIfNeeded used to check server.lua_caller which is NULL
at this point) but not EXEC (because server.in_eval==1)
This only affects modules as module.c the only user of server.in_eval.
Affects versions 6.2.0, 6.2.1
On a replica we do accept connections, even though commands accessing
the database will operate in read-only mode. But the server is still
already operational and processing commands.
Not sending the readiness notification means that on a HA setup where
the nodes all start as replicas (with replicaof in the config) with
a replica that cannot connect to the master server and which might not
come back in a predictable amount of time or at all, the service
supervisor will end up timing out the service and terminating it, with
no option to promote it to be the main instance. This seems counter to
what the readiness notification is supposed to be signaling.
Instead send the readiness notification when we start accepting
commands, and then send the various server status changes as that.
Fixes: commit 641c64ada1
Fixes: commit dfb598cf33
Bug 1:
When a module ctx is freed moduleHandlePropagationAfterCommandCallback
is called and handles propagation. We want to prevent it from propagating
commands that were not replicated by the same context. Example:
1. module1.foo does: RM_Replicate(cmd1); RM_Call(cmd2); RM_Replicate(cmd3)
2. RM_Replicate(cmd1) propagates MULTI and adds cmd1 to also_propagagte
3. RM_Call(cmd2) create a new ctx, calls call() and destroys the ctx.
4. moduleHandlePropagationAfterCommandCallback is called, calling
alsoPropagates EXEC (Note: EXEC is still not written to socket),
setting server.in_trnsaction = 0
5. RM_Replicate(cmd3) is called, propagagting yet another MULTI (now
we have nested MULTI calls, which is no good) and then cmd3
We must prevent RM_Call(cmd2) from resetting server.in_transaction.
REDISMODULE_CTX_MULTI_EMITTED was revived for that purpose.
Bug 2:
Fix issues with nested RM_Call where some have '!' and some don't.
Example:
1. module1.foo does RM_Call of module2.bar without replication (i.e. no '!')
2. module2.bar internally calls RM_Call of INCR with '!'
3. at the end of module1.foo we call RM_ReplicateVerbatim
We want the replica/AOF to see only module1.foo and not the INCR from module2.bar
Introduced a global replication_allowed flag inside RM_Call to determine
whether we need to replicate or not (even if '!' was specified)
Other changes:
Split beforePropagateMultiOrExec to beforePropagateMulti afterPropagateExec
just for better readability
Have a clear separation between in and out flags
Other changes:
delete dead code in RM_ZsetIncrby: if zsetAdd returned error (happens only if
the result of the operation is NAN or if score is NAN) we return immediately so
there is no way that zsetAdd succeeded and returned NAN in the out-flags
`master_sync_perc` and `loading_loaded_perc` don't have that sign,
and i think the info field should be a raw floating point number (the name suggests its units).
we already have `used_memory_peak_perc` and `used_memory_dataset_perc` which do add the `%` sign, but:
1) i think it was a mistake but maybe too late to fix now, and maybe not too late to fix for `current_fork_perc`
2) it is more important to be consistent with the two other "progress" "prec" metrics, and not with the "utilization" metric.
1. Add `redis-server test all` support to run all tests.
2. Add redis test to daily ci.
3. Add `--accurate` option to run slow tests for more iterations (so that
by default we run less cycles (shorter time, and less prints).
4. Move dict benchmark to REDIS_TEST.
5. fix some leaks in tests
6. make quicklist tests run on a specific fill set of options rather than huge ranges
7. move some prints in quicklist test outside their loops to reduce prints
8. removing sds.h from dict.c since it is now used in both redis-server and
redis-cli (uses hiredis sds)
The obtained process_rss was incorrect (the OS reports pages, not
bytes), resulting with many other fields getting corrupted.
This has been tested on FreeBSD but not other platforms.
When a quicklist has quicklist->compress * 2 nodes, then call
__quicklistCompress, all nodes will be decompressed and the middle
two nodes will be recompressed again. This violates the fact that
quicklist->compress * 2 nodes are uncompressed. It's harmless
because when visit a node, we always try to uncompress node first.
This only happened when a quicklist has quicklist->compress * 2 + 1
nodes, then delete a node. For other scenarios like insert node and
iterate this will not happen.
Scenario:
1. A module client is blocked on keys with a timeout
2. Shortly before the timeout expires, the key is being populated and signaled
as ready
3. Redis calls moduleTryServeClientBlockedOnKey (which replies to client) and
then moduleUnblockClient
4. moduleUnblockClient doesn't really unblock the client, it writes to
server.module_blocked_pipe and only marks the BC as unblocked.
5. beforeSleep kics in, by this time the client still exists and techincally
timed-out. beforeSleep replies to the timeout client (double reply) and
only then moduleHandleBlockedClients is called, reading from module_blocked_pipe
and calling unblockClient
The solution is similar to what was done in moduleTryServeClientBlockedOnKey: we
should avoid re-processing an already-unblocked client
* The `redis-cli --scan` output should honor output mode (set explicitly or implicitly), and quote key names when not in raw mode.
* Technically this is a breaking change, but it should be very minor since raw mode is by default on for non-tty output.
* It should only affect TTY output (human users) or non-tty output if `--no-raw` is specified.
* Added `--quoted-input` option to treat all arguments as potentially quoted strings.
* Added `--quoted-pattern` option to accept a potentially quoted pattern.
Unquoting is applied to potentially quoted input only if single or double quotes are used.
Fixes#8561, #8563
When the length of the quicklist is 1(only one zipmap), the rotate operation will cause
memory overlap when moving an entity from the tail of the zipmap to the head.
quicklistRotate is a dead code, so it has no impact on the existing code.
Since the API declared (as #define) in redismodule.h and uses
the CLIENT_ID_AOF that declared in the server.h, when
a module will want to make use of this API, it will get a compilation
error (module doesn't include the server.h).
The API was broken by d6eb3af (failed attempt for a cleanup).
Revert to the original version of RedisModule_IsAOFClient
that uses UINT64_MAX instead of CLIENT_ID_AOF
When no connected clients variables stopped being updated instead of being zeroed over time.
The other changes are cleanup and optimization (avoiding an unnecessary division per client)
When sanitizing the stream listpack, we need to count the deleted records too.
otherwise the last line that checks the next pointer fails.
Add test to cover that state in the stream tests.
Add ability to modify port, tls-port and bind configurations by CONFIG SET command.
To simplify the code and make it cleaner, a new structure
added, socketFds, which contains the file descriptors array and its counter,
and used for TCP, TLS and Cluster sockets file descriptors.
Because when the RM_Call is invoked. It will create a faker client.
The point is client connection is NULL, so server will crash in connGetInfo
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
The arm_thread_state64_get_pc used later in the file is defined
in mach kernel headers. Apparently they get included if you use
the system malloc but not if you use jemalloc.
DB ID used to be parsed as a long for SWAPDB command, now
make it into an int to be consistent with other commands that parses
the DB ID argument like SELECT, MOVE, COPY. See #8085
The implication is that the error message when the provided db index
is greater than 4M changes slightly.
This could happen on an invalid use, when trying to create a cluster with
a single node and provide it's address 3 time to satisfy redis-cli requirements.
A single client pointer is added in the server struct. This is
initialized by the first RM_Call() and reused for every subsequent
RM_Call() except if it's already in use, which means that it's not
used for (recursive) module calls to modules. For these, a new
"fake" client is created each time.
Other changes:
* Avoid allocating a dict iterator in pubsubUnsubscribeAllChannels
when not needed
* Add better control of malloc_usable_size() usage.
* Use malloc_usable_size on alpine libc daily job.
* Add no-malloc-usable-size daily jobs.
* Fix zmalloc(0) when HAVE_MALLOC_SIZE is undefined.
In order to align with the jemalloc behavior, this should never return
NULL or OOM panic.
This commit fixes a bug in what's currently dead code in redis.
In quicklistDelRange when delete entry from entry.offset to node tail,
extent only need gte node->count - entry.offset, not node->count
Co-authored-by: Yoav Steinberg <yoav@redislabs.com>
* Remove linux/version.h dependency.
This introduces unnecessary dependencies, and generally not a good idea
as the platform we build on may be different than the platform we run
on.
To determine if sync_file_range exists we can simply rely on header file
hints.
* Fix setproctitle() on libmusl.
The previous ifdef checks were a bit too strict for no apparent
reason.
* Fix tests failure on Linux with no backtrace.
* Add alpine daily CI job.
On 32-bit systems, setting the proto-max-bulk-len config parameter to a high value may result with integer overflow and a subsequent heap overflow when parsing an input bulk (CVE-2021-21309).
This fix has two parts:
Set a reasonable limit to the config parameter.
Add additional checks to prevent the problem in other potential but unknown code paths.
This validation was only done for sub-commands and not for commands.
These would have been valid (not produce any error)
ACL SETUSER bob +@all +client
ACL SETUSER bob +client +client
so no reason for this one to fail:
ACL SETUSER bob +client +client|id
One example why this is needed is that pfdebug wasn't part of the @hyperloglog
group and now it is. so something like:
acl setuser user1 +@hyperloglog +pfdebug|test
would have succeeded in early 6.0.x, and fail in 6.2 RC3
Co-authored-by: Harkrishn Patro <harkrisp@amazon.com>
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
Co-authored-by: Oran Agra <oran@redislabs.com>
SRANDMEMBER with negative count (non unique) can return the same member
multiple times, and the order of elements in the returned collection matters.
For these reasons returning a RESP3 Set type is not valid for the negative
count, but also not really valid for the positive (unique) variant either (the
command returns an array of random picks, not a set)
This PR also contains a minor optimization for SRANDMEMBER, HRANDFIELD,
and ZRANDMEMBER, to avoid the temporary dict from being rehashed while it grows.
Co-authored-by: Oran Agra <oran@redislabs.com>
Originally this was limited to IPv6 address length, but effectively it
has been used for host names and now that Sentinel accepts that as well
we need to be able to store full hostnames.
Fixes#8507
When redis responds with tracking-redir-broken push message (RESP3),
it was responding with a broken protocol: an array of 3 elements, but only
pushes 2 elements.
Some bugs in the test make this pass. Read the push reply
will consume an extra reply, because the reply length is 3, but there
are only two elements, so the next reply will be treated as third
element. So the test is corrected too.
Other changes:
* checkPrefixCollisionsOrReply success should return 1 instead of -1,
this bug didn't have any implications.
* improve client tracking tests to validate more of the response it reads.
Respond with error if expire time overflows from positive to negative of vice versa.
* `SETEX`, `SET EX`, `GETEX` etc would have already error on negative value,
but now they would also error on overflows (i.e. when the input was positive but
after the manipulation it becomes negative, which would have passed before)
* `EXPIRE` and `EXPIREAT` was ok taking negative values (would implicitly delete
the key), we keep that, but we do error if the user provided a value that changes
sign when manipulated (except the case of changing sign when `basetime` is added)
Signed-off-by: Gnanesh <gnaneshkunal@outlook.com>
Co-authored-by: Oran Agra <oran@redislabs.com>
The `dict` field `iterators` is misleading and incorrect.
This variable is used for 1 purpose - to pause rehashing.
The current `iterators` field doesn't actually count "iterators".
It counts "safe iterators". But - it doesn't actually count safe iterators
either. For one, it's only incremented once the safe iterator begins to
iterate, not when it's created. It's also incremented in `dictScan` to
prevent rehashing (and commented to make it clear why `iterators` is
being incremented during a scan).
This update renames the field as `pauserehash` and creates 2 helper
macros `dictPauseRehashing(d)` and `dictResumeRehashing(d)`
Avoid repeated reallocs growing the listpack while entries are being added.
This is done by pre-allocating the listpack to near maximum size, and using
malloc_size to check if it needs realloc or not.
When the listpack reaches the maximum number of entries, we shrink it to fit it's used size.
Co-authored-by: Viktor Söderqvist <viktor@zuiderkwast.se>
Co-authored-by: Oran Agra <oran@redislabs.com>
* Adding current_save_keys_total and current_save_keys_processed info fields.
Present in replication, BGSAVE and AOFRW.
* Changing RM_SendChildCOWInfo() to RM_SendChildHeartbeat(double progress)
* Adding new info field current_fork_perc. Present in Replication, BGSAVE, AOFRW,
and module forks.
Avoids memmove and reallocs when replacing a ziplist element of the
same encoded size as the new value.
Affects HSET, HINRBY, HINCRBYFLOAT (via hashTypeSet) and LSET (via
quicklistReplaceAtIndex).
The added flag affects the return value of RM_HashSet() to include
the number of inserted fields, in addition to updated and deleted
fields.
errno is set on errors, tests are added and documentation updated.
Fix the pointers to the slot hash tags in the case of prefixed commands usage
i.e. AUTH / SELECT
It adjusts the pointers to the slot hash tags in the case of prefixed commands
usage as soon as we get the 1st reply (same like we already did for the random
strings within the command )
We need to store replicas referenced by their announced address (IP or
address). Before that, if hostnames were used and the IP address
changed, duplicate entries would have been created.
- removes time sensitive checks from block on background tests during leak checks.
- fix uninitialized variable on RedisModuleBlockedClient() when calling
RM_BlockedClientMeasureTimeEnd() without RM_BlockedClientMeasureTimeStart()
1. Rename 18-cluster-nodes-slots.tcl to 19-cluster-nodes-slots.tcl.
it was conflicting with another test prefixed by 18
2. Release memory on exit in redis-cli.c.
3. Fix freeConvertedSds indentation.
When (remaining == (total_size - index)), element will definitely be random to.
But when rand() == RAND_MAX, the element will miss, this will trigger assert
in serverAssert(ziplistRandomPairsUnique(zsetobj->ptr, count, keys, vals) == count).
It is inefficient to repeatedly pick a single random element from a
ziplist.
For CASE4, which is when the user requested a low number of unique
random picks from the collectoin, we used thta pattern.
Now we use a different algorithm that picks unique elements from a
ziplist, and guarentee no duplicate but doesn't provide random order
(which is only needed in the non-unique random picks case)
Unrelated changes:
* change ziplist count and indexes variables to unsigned
* solve compilation warnings about uninitialized vars in gcc 10.2
Co-authored-by: xinluton <xinluton@qq.com>
Disable certificate validation, making it possible to connect to servers
without configuring full trust chain.
The use of this option is insecure and makes the connection vulnerable
to man in the middle attacks.
Without this fix, RM_ZsetRem can leave empty sorted sets which are
not allowed to exist.
Removing from a sorted set while iterating seems to work (while
inserting causes failed assetions). RM_ZsetRangeEndReached is
modified to return 1 if the key doesn't exist, to terminate
iteration when the last element has been removed.
Changes to HRANDFIELD and ZRANDMEMBER:
* Fix risk of OOM panic when client query a very big negative count (avoid allocating huge temporary buffer).
* Fix uneven random distribution in HRANDFIELD with negative count (wasn't using dictGetFairRandomKey).
* Add tests to check an even random distribution (HRANDFIELD, SRANDMEMBER, ZRANDMEMBER).
Co-authored-by: Oran Agra <oran@redislabs.com>
Fix errors of GEOSEARCH bybox search due to:
1. projection of the box to a trapezoid (when the meter box is converted to long / lat it's no longer a box).
2. width and height mismatch
Changes:
- New GEOSEARCH point in rectangle algorithm
- Fix GEOSEARCH bybox width and height mismatch bug
- Add GEOSEARCH bybox testing to the existing "GEOADD + GEORANGE randomized test"
- Add new fuzzy test to stress test the bybox corners and edges
- Add some tests for edge cases of the bybox algorithm
Co-authored-by: Oran Agra <oran@redislabs.com>
This commit provides an optimization, in terms of time, for all GEORADIUS*
and GEOSEARCH* searches which utilize the default, sorted, COUNT clause.
This is commonly used for nearest-neighbor (top-K points closest to a given lat/lon)
searches. While the previous implementation appends all matching points to the
geoPoint array and performs pruning after-the-fact via a full sort and [0, count)-based
for-loop, this PR sorts only the required number of elements.
This optimization provides a 5-20% improvement in runtime depending on the
density of points of interest (POI) as well as the radius searched.
No performance degradation has been observed.
addReplyLongLongWithPrefix, has a check against negative length, and the code
flow removed in this commit bypasses the check.
addReplyAggregateLen has an assertion for negative length, but addReplyBulkLen
does not, so this commit fixes theoretical case of access violation (probably
unreachable though)
* The corrupt dump fuzzer found a division by zero.
* in some cases the random fields from the HRANDFIELD tests produced
fields with newlines and other special chars (due to \ char), this caused
the TCL tests to see a bulk response that has a newline in it and add {}
around it, later it can think this is a nested list. in fact the `alpha` random
string generator isn't using spaces and newlines, so it should not use `\`
either.
This commit enables tracking time of the background tasks and on replies,
opening the door for properly tracking commands that rely on blocking / background
work via the slowlog, latency history, and commandstats.
Some notes:
- The time spent blocked waiting for key changes, or blocked on synchronous
replication is not accounted for.
- **This commit does not affect latency tracking of commands that are non-blocking
or do not have background work.** ( meaning that it all stays the same with exception to
`BZPOPMIN`,`BZPOPMAX`,`BRPOP`,`BLPOP`, etc... and module's commands that rely
on background threads ).
- Specifically for latency history command we've added a new event class named
`command-unblocking` that will enable latency monitoring on commands that spawn
background threads to do the work.
- For blocking commands we're now considering the total time of a command as the
time spent on call() + the time spent on replying when unblocked.
- For Modules commands that rely on background threads we're now considering the
total time of a command as the time spent on call (main thread) + the time spent on
the background thread ( if marked within `RedisModule_MeasureTimeStart()` and
`RedisModule_MeasureTimeEnd()` ) + the time spent on replying (main thread)
To test for this feature we've added a `unit/moduleapi/blockonbackground` test that relies on
a module that blocks the client and sleeps on the background for a given time.
- check blocked command that uses RedisModule_MeasureTimeStart() is tracking background time
- check blocked command that uses RedisModule_MeasureTimeStart() is tracking background time even in timeout
- check blocked command with multiple calls RedisModule_MeasureTimeStart() is tracking the total background time
- check blocked command without calling RedisModule_MeasureTimeStart() is not reporting background time
New commands:
`HRANDFIELD [<count> [WITHVALUES]]`
`ZRANDMEMBER [<count> [WITHSCORES]]`
Algorithms are similar to the one in SRANDMEMBER.
Both return a simple bulk response when no arguments are given, and an array otherwise.
In case values/scores are requested, RESP2 returns a long array, and RESP3 a nested array.
note: in all 3 commands, the only option that also provides random order is the one with negative count.
Changes to SRANDMEMBER
* Optimization when count is 1, we can use the more efficient algorithm of non-unique random
* optimization: work with sds strings rather than robj
Other changes:
* zzlGetScore: when zset needs to convert string to double, we use safer memcpy (in
case the buffer is too small)
* Solve a "bug" in SRANDMEMBER test: it intended to test a positive count (case 3 or
case 4) and by accident used a negative count
Co-authored-by: xinluton <xinluton@qq.com>
Co-authored-by: Oran Agra <oran@redislabs.com>
The key point is how to recover from last AOF write error, for example:
1. start redis with appendonly yes, and append some write commands
2. short write or something else error happen, `server.aof_last_write_status` changed to `C_ERR`, now redis doesn't accept write commands
3. execute `CONFIG SET appendonly no` to avoid the above problem, now redis can accept write commands again
4. disk error resolved, and execute `CONFIG SET appendonly yes` to reopen AOF, but `server.aof_last_write_status` cannot be changed to `C_OK` (if background aof rewrite run less then 1 second, it will free `server.aof_buf` and then serverCron cannot fix `aof_last_write_status`), then redis cannot accept write commands forever.
This PR use a simple way to fix it:
1. just free `server.aof_buf` when stop appendonly to save memory, if error happens in `flushAppendOnlyFile(1)`, the `server.aof_buf` may contains some data which has not be written to aof, I think we can ignore it because we turn off the appendonly.
2. reset fsync status after stop appendonly and call `flushAppendOnlyFile` only when `aof_state` is ON
3. reset `server.last_write_status` when reopen aof to accept write commands
