Commit Graph

11470 Commits

Author SHA1 Message Date
Wen Hui
2f411770c8
Add CONFIG SET and GET loglevel feature in Sentinel (#11214)
Till now Sentinel allowed modifying the log level in the config file, but not at runtime.
this makes it possible to tune the log level at runtime
2022-11-20 12:03:00 +02:00
Ping Xie
203b12e41f
Introduce Shard IDs to logically group nodes in cluster mode (#10536)
Introduce Shard IDs to logically group nodes in cluster mode.
1. Added a new "shard_id" field to "cluster nodes" output and nodes.conf after "hostname"
2. Added a new PING extension to propagate "shard_id"
3. Handled upgrade from pre-7.2 releases automatically
4. Refactored PING extension assembling/parsing logic

Behavior of Shard IDs:

Replicas will always follow the shards of their reported primaries. If a primary updates its shard ID, the replica will follow. (This need not follow for cluster v2) This is not an expected use case.
2022-11-16 19:24:18 -08:00
sundb
2168ccc661
Add listpack encoding for list (#11303)
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.
2022-11-16 20:29:46 +02:00
Madelyn Olson
d136bf2830
Explicitly send function commands to monitor (#11510)
Both functions and eval are marked as "no-monitor", since we want to explicitly feed in the script command before the commands generated by the script. Note that we want this behavior generally, so that commands can redact arguments before being added to the monitor.
2022-11-15 17:21:27 -08:00
Binbin
a4bcdbcfd3
Fix double negative nan test, ignoring sign (#11506)
The test introduced in #11482 fail on ARM (extra CI):
```
*** [err]: RESP2: RM_ReplyWithDouble: NaN in tests/unit/moduleapi/reply.tcl
Expected '-nan' to be equal to 'nan' (context: type eval line 3 cmd
{assert_equal "-nan" [r rw.double 0 0]} proc ::test)

*** [err]: RESP3: RM_ReplyWithDouble: NaN in tests/unit/moduleapi/reply.tcl
Expected ',-nan' to be equal to ',nan' (context: type eval line 8 cmd
{assert_equal ",-nan" [r rw.double 0 0]} proc ::test)
```

It looks like there is no negative nan on ARM.
2022-11-15 17:18:21 +02:00
uriyage
e4eb18b303
Module CLIENT_CHANGE, Fix crash on free blocked client with DB!=0 (#11500)
In moduleFireServerEvent we change the real client DB to 0 on freeClient in case the event is REDISMODULE_EVENT_CLIENT_CHANGE.
It results in a crash if the client is blocked on a key on other than DB 0.

The DB change is not necessary even for module-client, as we set its DB to 0 on either createClient or moduleReleaseTempClient.

Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
2022-11-14 14:40:35 -08:00
Binbin
2a2e5d416a
Fix double inf test, use readraw to verify the protocol (#11504)
The test introduced in #11482 fail on mac:
```
*** [err]: RESP3: RM_ReplyWithDouble: inf in tests/unit/moduleapi/reply.tcl
Expected 'Inf' to be equal to 'inf'
(context: type eval line 6 cmd {assert_equal Inf [r rw.double inf]} proc ::test)
```

Looks like the mac platform returns inf instead of Inf in this case, this PR
uses readraw to verify the protocol.
2022-11-14 11:07:10 +02:00
Oran Agra
78dc292178
Add test to cover NAN reply using a module (#11482)
Adding a test to cover the already existing behavior of NAN replies,
to accompany the PR that adds them to the RESP3 spec:
https://github.com/redis/redis-specifications/pull/10

This PR also covers Inf replies that are already in the spec, as well as RESP2 coverage.
2022-11-13 13:12:22 +02:00
Oran Agra
4c54528f0f
fixes for fork child exit and test: #11463 (#11499)
Fix a few issues with the recent #11463
* use exitFromChild instead of exit
* test should ignore defunct process since that's what we expect to
  happen for thees child processes when the parent dies.
* fix typo

Co-authored-by: Binbin <binloveplay1314@qq.com>
2022-11-12 20:35:34 +02:00
Binbin
6617f1704b
Add missing lpFree for listpack test, fix valgrind daily (#11492)
Add missing lpFree, introduced in #11290
2022-11-10 10:27:38 +02:00
Viktor Söderqvist
4e472a1a7f
Listpack encoding for sets (#11290)
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.
2022-11-09 19:50:07 +02:00
Viktor Söderqvist
07d187066a
Deprecate QUIT (#11439)
Clients should not use this command.
Instead, clients should simply close the connection when they're not used anymore.
Terminating a connection on the client side is preferable, as it eliminates `TIME_WAIT`
lingering sockets on the server side.
2022-11-09 14:41:00 +02:00
Oran Agra
ccaef5c923
diskless master, avoid bgsave child hung when fork parent crashes (#11463)
During a diskless sync, if the master main process crashes, the child would
have hung in `write`. This fix closes the read fd on the child side, so that if the
parent crashes, the child will get a write error and exit.

This change also fixes disk-based replication, BGSAVE and AOFRW.
In that case the child wouldn't have been hang, it would have just kept
running until done which may be pointless.

There is a certain degree of risk here. in case there's a BGSAVE child that could
maybe succeed and the parent dies for some reason, the old code would have let
the child keep running and maybe succeed and avoid data loss.
On the other hand, if the parent is restarted, it would have loaded an old rdb file
(or none), and then the child could reach the end and rename the rdb file (data
conflicting with what the parent has), or also have a race with another BGSAVE
child that the new parent started.

Note that i removed a comment saying a write error will be ignored in the child
and handled by the parent (this comment was very old and i don't think relevant).
2022-11-09 10:02:18 +02:00
Ozan Tezcan
f928991853
Tag test with needs:save (#11485)
Add needs:save tag for the test introduced by #11376
2022-11-08 14:58:38 +02:00
Oran Agra
3e112d4610
Use jemalloc by default also on ARM (#11407)
Till now Redis attempted to avoid using jemalloc on ARM, but didn't do that properly (missing armv8l and aarch64), so in fact we did you jemalloc on these without a problem.

Side notes:

Some ARM platforms, which share instruction set and can share binaries (docker images), may have different page size, and apparently jemalloc uses the page size of the build machine as the maximum page size to be supported by the build.
see https://github.com/redis-stack/redis-stack/issues/187

To work around that, when building for ARM, one can change the maximum page size to 64k (or greater if present on the build machine) In recent versions of jemalloc, this should not have any severe side effects (like VM map fragmentation), see:
https://github.com/jemalloc/jemalloc/issues/467
https://github.com/redis/redis/pull/11170#issuecomment-1236265230

To do that, one can use:
```
JEMALLOC_CONFIGURE_OPTS="--with-lg-page=16" make
```

Besides that, this PR fixes a messy makefile condition that was created
here: f30b18f4de
2022-11-07 19:11:12 +02:00
yancz2000
1f2456389b
Unify repeated code in redis-check-aof (#11456) 2022-11-06 14:49:55 +02:00
Hanif Ariffin
0cbe10e892
Remove redundant calls to update refcount of shared integers (#11479)
Since they're created with makeObjectShared, then incrRefCount on them is a NOP.

Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>
2022-11-06 08:33:28 +02:00
Binbin
5246bf4544
Bump vmactions/freebsd-vm to 0.3.0 to fix FreeBSD daily (#11476)
Our FreeBSD daily has been failing recently:
```
  Config file: freebsd-13.1.conf
  cd: /Users/runner/work/redis/redis: No such file or directory
  gmake: *** No targets specified and no makefile found.  Stop.
```

Upgrade vmactions/freebsd-vm to the latest version (0.3.0) can work.
I've tested it, but don't know why, but first let's fix it.
2022-11-04 20:28:27 +02:00
Binbin
fac188b49d
Introduce socket shutdown into connection type, used if a fork is active (#11376)
Introduce socket `shutdown()` into connection type, and use it
on normal socket if a fork is active. This allows us to close
client connections when there are child processes sharing the
file descriptors.

Fixes #10077. The reason is that since the `fork()` child is holding
the file descriptors, the `close` in `unlinkClient -> connClose`
isn't sufficient. The client will not realize that the connection is
disconnected until the child process ends.

Let's try to be conservative and only use shutdown when the fork is active.
2022-11-04 18:46:37 +02:00
Madelyn Olson
c337c0a8a4
Retain ACL categories used to generate ACL for displaying them later (#11224)
Retain ACL categories used to generate ACL for displaying them later
2022-11-03 10:14:56 -07:00
Binbin
8764611c8a
Block some specific characters in module command names (#11434)
Today we don't place any specific restrictions on module command names.
This can cause ambiguous scenarios. For example, someone might name a
command like "module|feature" which would be incorrectly parsed by the
ACL system as a subcommand.

In this PR, we will block some chars that we know can mess things up.
Specifically ones that can appear ok at first and cause problems in some
cases (we rather surface the issue right away).

There are these characters:
 * ` ` (space) - issues with old inline protocol.
 * `\r`, `\n` (newline) - can mess up the protocol on acl error replies.
 * `|` - sub-commands.
 * `@` - ACL categories
 * `=`, `,` - info and client list fields.

note that we decided to leave `:` out as it's handled by `getSafeInfoString`
and is more likely to already been used by existing modules.
2022-11-03 13:19:49 +02:00
Wen Hui
7395e370e6
Fix XSETID with max_deleted_entry_id issue (#11444)
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**
2022-11-02 16:16:16 +02:00
Wen Hui
fea9bbbe0f
Fix command BITFIELD_RO and BITFIELD argument json file, add some test cases for them (#11445)
According to the source code, the commands can be executed with only key name,
and no GET/SET/INCR operation arguments.
change the docs to reflect that by marking these arguments as optional.
also add tests.
2022-11-02 15:15:12 +02:00
Binbin
e632e62e68
Print IP and port on cluster bus message sanity check (#11443)
* Print IP and port on cluster bus message sanity check

Add a print statement to indicate which IP/port is sending
the error messages. That way we can at least check to see
if it is a node in the cluster or some other nefarious nodes.

It is proposed in #11339.

Unrelated changes: the return check for connAddrPeerName should
be -1 instead of C_ERR, although the value of C_ERR is also -1.

Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com>
2022-11-01 19:27:30 -07:00
Brennan
47c493e070
Re-design cluster link send buffer to improve memory management (#11343)
Re-design cluster link send queue to improve memory management
2022-11-01 19:26:44 -07:00
Wen Hui
4a8a625051
add test case for geopos and geohash (#11455)
This PR add test case for PR #11417, with only key as argument for GEOHASH and GEOPOS
2022-11-01 07:54:03 +02:00
xbasel
5b102e2339
Don't process file events if AE_FILE_EVENTS isn't set. (#11428)
All current usage of the function are correct, but in the future it might change.
2022-10-29 16:24:12 -07:00
Moti Cohen
c0d7226274
Refactor and (internally) rebrand from pause-clients to pause-actions (#11098)
Renamed from "Pause Clients" to "Pause Actions" since the mechanism can pause
several actions in redis, not just clients (e.g. eviction, expiration).

Previously each pause purpose (which has a timeout that's tracked separately from others purposes),
also implicitly dictated what it pauses (reads, writes, eviction, etc). Now it is explicit, and
the actions that are paused (bit flags) are defined separately from the purpose.

- Previously, when using feature pause-client it also implicitly means to make the server static:
  - Pause replica traffic
  - Pauses eviction processing
  - Pauses expire processing

Making the server static is used also for failover and shutdown. This PR internally rebrand
pause-client API to become pause-action API. It also Simplifies pauseClients structure
by replacing pointers array with static array.

The context of this PR is to add another trigger to pause-client which will activated in case
of OOM as throttling mechanism ([see here](https://github.com/redis/redis/issues/10907)).
In this case we want only to pause client, and eviction actions.
2022-10-27 11:57:04 +03:00
Shaya Potter
38028dab8d
RM_Call - only enforce OOM on scripts if 'M' flag is sent (#11425)
RM_Call is designed to let modules call redis commands disregarding the
OOM state (the module is responsible to declare its command flags to redis,
or perform the necessary checks).
The other (new) alternative is to pass the "M" flag to RM_Call so that redis can
OOM reject commands implicitly.

However, Currently, RM_Call enforces OOM on scripts (excluding scripts that
declared `allow-oom`) in all cases, regardless of the RM_Call "M" flag being present.

This PR fixes scripts to be consistent with other commands being executed by RM_Call.
It modifies the flow in effect treats scripts as if they if they have the ALLOW_OOM script
flag, if the "M" flag is not passed (i.e. no OOM checking is being performed by RM_Call,
so no OOM checking should be done on script).

Co-authored-by: Oran Agra <oran@redislabs.com>
2022-10-27 09:29:43 +03:00
AntiTopQuark
af43617122
fix: reducing duplicate header file definitions (#11437)
Reducing duplicate header file definitions <sys/resource.h>
2022-10-26 08:45:19 -07:00
xbasel
e9d4ed4e0f
Remove unit test pendingquerybuf.tcl since pending_querybuf no longer exists. (#11429) 2022-10-25 14:36:49 -07:00
Wen Hui
7c3916ae6e
Fix command GEOHASH and GEOPOS argument doc, mark member as optional (#11417)
These commands take a list of members, which can be empty (i.e. running
the command with just a key name).
this always results in an empty array reply, so it doesn't make much sense,
but changing it is a breaking change.

This PR fixes the documentation, making the member field as optional, just makes
sure the command format documentation is consistent with the command behavior.

The command format will be:

127.0.0.1:6381> GEOPOS key [member [member ...]]
127.0.0.1:6381> GEOHASH key [member [member ...]]
2022-10-25 14:26:08 +03:00
guybe7
f8970fdbfa
Cleanup: Remove redundant arg from moduleCreateArgvFromUserFormat (#11426)
We do not need to return the length of argv because it is equal to argc, which we return anyway.
This change makes the code cleaner and adds a comment to explain something that might not be immediately clear.
2022-10-24 18:30:16 +03:00
guybe7
737a090511
Set errno in case XADD with partial ID fails (#11424)
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`).
2022-10-24 18:27:56 +03:00
Moti Cohen
bd23b15ad7
Fix sentinel function that compares hostnames (if failed resolve) (#11419)
Funcion sentinelAddrEqualsHostname() of sentinel makes DNS resolve
and based on it determines if two IP addresses are equal. Now, If the
DNS resolve command fails, the function simply returns 0, even if the
hostnames are identical.

This might become an issue in case of failover such that sentinel might
receives from Redis instance, response to regular INFO query it sent,
and wrongly decide that the instance is pointing to is different leader
than the one recorded because of this function, yet hostnames are
identical. In turn sentinel disconnects the connection between sentinel
and valid slave which leads to -failover-abort-no-good-slave.
See issue #11241.

I managed to reproduce only part of the flow in which the function
return wrong result and trigger +fix-slave-config.

The fix is even if the function failed to resolve then compare based on
hostnames. That is our best effort as long as the server is unavailable
for some reason. It is fine since Redis instance cannot have multiple
hostnames for a given setup
2022-10-23 16:00:47 +03:00
qetu3790
20df1424a2
Fix wrong tips when the user pass wrong # of arguments to redis.set_repl(). (#11415)
redis.set_repl() needs one arg, but the tips says two.
2022-10-23 15:06:58 +03:00
Binbin
9e1b879f5b
Make PFMERGE source key optional in docs, add tests with one input key, add tests on missing source keys (#11205)
The following two cases will create an empty destkey HLL:
1. called with no source keys, like `pfmerge destkey`
2. called with non-existing source keys, like `pfmerge destkey non-existing-source-key`

In the first case, in `PFMERGE`, the dest key is actually one of the source keys too.
So `PFMERGE k1 k2` is equivalent to `SUNIONSTORE k1 k1 k2`,
and `PFMERGE k1` is equivalent to `SUNIONSTORE k1 k1`.
So the first case is reasonable, the source key is actually optional.

And the second case, `PFMERGE` on missing keys should succeed and create an empty dest.
This is consistent with `PFCOUNT`, and also with `SUNIONSTORE`, no need to change.
2022-10-22 20:41:17 +03:00
sundb
6dd213558b
Fix crash due to to reuse iterator entry after list deletion in module (#11383)
In the module, we will reuse the list iterator entry for RM_ListDelete, but `listTypeDelete` will only update
`quicklistEntry->zi` but not `quicklistEntry->node`, which will result in `quicklistEntry->node` pointing to
a freed memory address if the quicklist node is deleted. 

This PR sync `key->u.list.index` and `key->u.list.entry` to list iterator after `RM_ListDelete`.

This PR also optimizes the release code of the original list iterator.

Co-authored-by: Viktor Söderqvist <viktor@zuiderkwast.se>
2022-10-22 20:36:50 +03:00
FutabaRio
6debeb3779
fix the size of variable merge_sz in quicklist.c (#11285)
11 was the size of header/trailer in the old structure Ziplist,
but now the size of header/trailer in the new structure Listpack should be 7.
2022-10-19 15:11:28 +03:00
guybe7
b57fd01064
Blocked module clients should be aware when a key is deleted (#11310)
The use case is a module that wants to implement a blocking command on a key that
necessarily exists and wants to unblock the client in case the key is deleted (much like
what we implemented for XREADGROUP in #10306)

New module API:
* RedisModule_BlockClientOnKeysWithFlags

Flags:
* REDISMODULE_BLOCK_UNBLOCK_NONE
* REDISMODULE_BLOCK_UNBLOCK_DELETED

### Detailed description of code changes

blocked.c:
1. Both module and stream functions are called whether the key exists or not, regardless of
  its type. We do that in order to allow modules/stream to unblock the client in case the key
  is no longer present or has changed type (the behavior for streams didn't change, just code
  that moved into serveClientsBlockedOnStreamKey)
2. Make sure afterCommand is called in serveClientsBlockedOnKeyByModule, in order to propagate
  actions from moduleTryServeClientBlockedOnKey.
3. handleClientsBlockedOnKeys: call propagatePendingCommands directly after lookupKeyReadWithFlags
  to prevent a possible lazy-expire DEL from being mixed with any command propagated by the
  preceding functions.
4. blockForKeys: Caller can specifiy that it wants to be awakened if key is deleted.
   Minor optimizations (use dictAddRaw).
5. signalKeyAsReady became signalKeyAsReadyLogic which can take a boolean in case the key is deleted.
  It will only signal if there's at least one client that awaits key deletion (to save calls to
  handleClientsBlockedOnKeys).
  Minor optimizations (use dictAddRaw)

db.c:
1. scanDatabaseForDeletedStreams is now scanDatabaseForDeletedKeys and will signalKeyAsReady
  for any key that was removed from the database or changed type. It is the responsibility of the code
  in blocked.c to ignore or act on deleted/type-changed keys.
2. Use the new signalDeletedKeyAsReady where needed

blockedonkey.c + tcl:
1. Added test of new capabilities (FSL.BPOPGT now requires the key to exist in order to work)
2022-10-18 19:50:02 +03:00
Meir Shpilraien (Spielrein)
b43f254813
Avoid saving module aux on RDB if no aux data was saved by the module. (#11374)
### Background

The issue is that when saving an RDB with module AUX data, the module AUX metadata
(moduleid, when, ...) is saved to the RDB even though the module did not saved any actual data.
This prevent loading the RDB in the absence of the module (although there is no actual data in
the RDB that requires the module to be loaded).

### Solution

The solution suggested in this PR is that module AUX will be saved on the RDB only if the module
actually saved something during `aux_save` function.

To support backward compatibility, we introduce `aux_save2` callback that acts the same as
`aux_save` with the tiny change of avoid saving the aux field if no data was actually saved by
the module. Modules can use the new API to make sure that if they have no data to save,
then it will be possible to load the created RDB even without the module.

### Concerns

A module may register for the aux load and save hooks just in order to be notified when
saving or loading starts or completed (there are better ways to do that, but it still possible
that someone used it).

However, if a module didn't save a single field in the save callback, it means it's not allowed
to read in the read callback, since it has no way to distinguish between empty and non-empty
payloads. furthermore, it means that if the module did that, it must never change it, since it'll
break compatibility with it's old RDB files, so this is really not a valid use case.

Since some modules (ones who currently save one field indicating an empty payload), need
to know if saving an empty payload is valid, and if Redis is gonna ignore an empty payload
or store it, we opted to add a new API (rather than change behavior of an existing API and
expect modules to check the redis version)

### Technical Details

To avoid saving AUX data on RDB, we change the code to first save the AUX metadata
(moduleid, when, ...) into a temporary buffer. The buffer is then flushed to the rio at the first
time the module makes a write operation inside the `aux_save` function. If the module saves
nothing (and `aux_save2` was used), the entire temporary buffer is simply dropped and no
data about this AUX field is saved to the RDB. This make it possible to load the RDB even in
the absence of the module.

Test was added to verify the fix.
2022-10-18 19:45:46 +03:00
Shuning
20d286f77e
keyIsExpired checks server.loading before calling getExpire (#11393)
Seems excessive to call getExpire if we don't need it.
This can maybe have some speedup on AOF file loading (saving a dictFind call)

Co-authored-by: lvshuning <lvshuning@meituan.com>
2022-10-18 13:55:49 +03:00
DarrenJiang13
ba1f09d3fe
fix malloc macro in listpack.c (#11398)
fix some malloc macros in `listpack.c`.
listpack has it's own malloc aliases, but in some places normal redis malloc calls have slipped in.
2022-10-18 08:28:25 +03:00
Binbin
a9d561afa5
Bump codespell from 2.2.1 to 2.2.2 in /.codespell (#11399)
And fix a few newly detected typo.
Closes #11394
2022-10-18 08:24:30 +03:00
Shaya Potter
3193f086ca
Unify ACL failure error messaging. (#11160)
Motivation: for applications that use RM ACL verification functions, they would
want to return errors back to the user, in ways that are consistent with Redis.
While investigating how we should return ACL errors to the user, we realized that
Redis isn't consistent, and currently returns ACL error strings in 3 primary ways.

[For the actual implications of this change, see the "Impact" section at the bottom]

1. how it returns an error when calling a command normally
   ACL_DENIED_CMD -> "this user has no permissions to run the '%s' command"
   ACL_DENIED_KEY -> "this user has no permissions to access one of the keys used as arguments"
   ACL_DENIED_CHANNEL -> "this user has no permissions to access one of the channels used as arguments"

2. how it returns an error when calling via 'acl dryrun' command
   ACL_DENIED_CMD ->  "This user has no permissions to run the '%s' command"
   ACL_DENIED_KEY -> "This user has no permissions to access the '%s' key"
   ACL_DENIED_CHANNEL -> "This user has no permissions to access the '%s' channel"

3. how it returns an error via RM_Call (and scripting is similar).
   ACL_DENIED_CMD -> "can't run this command or subcommand";
   ACL_DENIED_KEY -> "can't access at least one of the keys mentioned in the command arguments";
   ACL_DENIED_CHANNEL -> "can't publish to the channel mentioned in the command";
   
   In addition, if one wants to use RM_Call's "dry run" capability instead of the RM ACL
   functions directly, one also sees a different problem than it returns ACL errors with a -ERR,
   not a -PERM, so it can't be returned directly to the caller.

This PR modifies the code to generate a base message in a common manner with the ability
to set verbose flag for acl dry run errors, and keep it unset for normal/rm_call/script cases

```c
sds getAclErrorMessage(int acl_res, user *user, struct redisCommand *cmd, sds errored_val, int verbose) {
    switch (acl_res) {
    case ACL_DENIED_CMD:
        return sdscatfmt(sdsempty(), "User %S has no permissions to run "
                                     "the '%S' command", user->name, cmd->fullname);
    case ACL_DENIED_KEY:
        if (verbose) {
            return sdscatfmt(sdsempty(), "User %S has no permissions to access "
                                         "the '%S' key", user->name, errored_val);
        } else {
            return sdsnew("No permissions to access a key");
        }
    case ACL_DENIED_CHANNEL:
        if (verbose) {
            return sdscatfmt(sdsempty(), "User %S has no permissions to access "
                                         "the '%S' channel", user->name, errored_val);
        } else {
            return sdsnew("No permissions to access a channel");
        }
    }
```

The caller can append/prepend the message (adding NOPERM for normal/RM_Call or indicating it's within a script).

Impact:
- Plain commands, as well as scripts and RM_Call now include the user name.
- ACL DRYRUN remains the only one that's verbose (mentions the offending channel or key name)
- Changes RM_Call ACL errors from being a `-ERR` to being `-NOPERM` (besides for textual changes)
  **This somewhat a breaking change, but it only affects the RM_Call with both `C` and `E`, or `D`**
- Changes ACL errors in scripts textually from being
  `The user executing the script <old non unified text>`
  to
  `ACL failure in script: <new unified text>`
2022-10-16 09:01:37 +03:00
Meir Shpilraien (Spielrein)
56f97bfa5f
Fix wrong replication on cluster slotmap changes with module KSN propagation (#11377)
As discussed on #11084, `propagatePendingCommands` should happened after the del
notification is fired so that the notification effect and the `del` will be replicated inside MULTI EXEC.

Test was added to verify the fix.
2022-10-16 08:30:01 +03:00
David CARLIER
871cc200a0
Fixes build warning when CACHE_LINE_SIZE is already defined. (#11389)
* Fixes build warning when CACHE_LINE_SIZE is already defined
* Fixes wrong CACHE_LINE_SIZE on some FreeBSD systems where it could be set to 128 (e.g. on MIPS)
* Fixes wrong CACHE_LINE_SIZE on Apple M1 (use 128 instead of 64)

Wrong cache line size in that case can some false sharing of array elements between threads, see #10892
2022-10-16 07:51:44 +03:00
filipe oliveira
29380ff77d
optimizing d2string() and addReplyDouble() with grisu2: double to string conversion based on Florian Loitsch's Grisu-algorithm (#10587)
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`)
2022-10-15 12:17:41 +03:00
C Charles
9ab873d9d3
MIGTATE with AUTH that contains "keys" is getting wrong key names in migrateGetKeys, leads to ACL errors (#11253)
When using the MIGRATE, with a destination Redis that has the user name or password set to the string "keys",
Redis would have determine the wrong set of key names the command is gonna access.
This lead to ACL returning wrong authentication result.

Destination instance:
```
127.0.0.1:6380> acl setuser default >keys
OK
127.0.0.1:6380> acl setuser keys on nopass ~* &* +@all
OK
```

Source instance:
```
127.0.0.1:6379> set a 123
OK
127.0.0.1:6379> acl setuser cc on nopass ~a* +@all
OK
127.0.0.1:6379> auth cc 1
OK
127.0.0.1:6379> migrate 127.0.0.1 6380 "" 0 1000 auth keys keys a
(error) NOPERM this user has no permissions to access one of the keys used as arguments
127.0.0.1:6379> migrate 127.0.0.1 6380 "" 0 1000 auth2 keys pswd keys a
(error) NOPERM this user has no permissions to access one of the keys used as arguments
```

Using `acl dryrun` we know that the parameters of `auth` and `auth2` are mistaken for the `keys` option.
```
127.0.0.1:6379> acl dryrun cc migrate whatever whatever "" 0 1000 auth keys keys a
"This user has no permissions to access the 'keys' key"
127.0.0.1:6379> acl dryrun cc migrate whatever whatever "" 0 1000 auth2 keys pswd keys a
"This user has no permissions to access the 'pswd' key"
```

Fix the bug by editing db.c/migrateGetKeys function, which finds the `keys` option and all the keys following.
2022-10-13 15:03:54 +03:00
Oran Agra
dd60c6c8d3
Improve linux overcommit check and warning (#11357)
1. show the overcommit warning when overcommit is disabled (2),
   not just when it is set to heuristic (0).
2. improve warning text to mention the issue with jemalloc causing VM
   mapping fragmentation when set to 2.
2022-10-13 13:05:20 +03:00