SCAN/RANDOMKEY and lazy-expire (#11788)
Starting from Redis 7.0 (#9890) we started wrapping everything a command propagates with MULTI/EXEC. The problem is that both SCAN and RANDOMKEY can lazy-expire arbitrary keys (similar behavior to active-expire), and put DELs in a transaction. Fix: When these commands are called without a parent exec-unit (e.g. not in EVAL or MULTI) we avoid wrapping their DELs in a transaction (for the same reasons active-expire and eviction avoids a transaction) This PR adds a per-command flag that indicates that the command may touch arbitrary keys (not the ones in the arguments), and uses that flag to avoid the MULTI-EXEC. For now, this flag is internal, since we're considering other solutions for the future. Note for cluster mode: if SCAN/RANDOMKEY is inside EVAL/MULTI it can still cause the same situation (as it always did), but it won't cause a CROSSSLOT because replicas and AOF do not perform slot checks. The problem with the above is mainly for 3rd party ecosystem tools that propagate commands from master to master, or feed an AOF file with redis-cli into a master. This PR aims to fix the regression in redis 7.0, and we opened #11792 to try to handle the bigger problem with lazy expire better for another release.
This commit is contained in:
guybe7
GitHub
parent
7dae142a2e
commit
fd82bccd0e
@ -7210,11 +7210,11 @@ struct redisCommand redisCommandTable[] = {
|
||||
{"pexpireat","Set the expiration for a key as a UNIX timestamp specified in milliseconds","O(1)","2.6.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_GENERIC,PEXPIREAT_History,PEXPIREAT_tips,pexpireatCommand,-3,CMD_WRITE|CMD_FAST,ACL_CATEGORY_KEYSPACE,{{NULL,CMD_KEY_RW|CMD_KEY_UPDATE,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}}},.args=PEXPIREAT_Args},
|
||||
{"pexpiretime","Get the expiration Unix timestamp for a key in milliseconds","O(1)","7.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_GENERIC,PEXPIRETIME_History,PEXPIRETIME_tips,pexpiretimeCommand,2,CMD_READONLY|CMD_FAST,ACL_CATEGORY_KEYSPACE,{{NULL,CMD_KEY_RO|CMD_KEY_ACCESS,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}}},.args=PEXPIRETIME_Args},
|
||||
{"pttl","Get the time to live for a key in milliseconds","O(1)","2.6.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_GENERIC,PTTL_History,PTTL_tips,pttlCommand,2,CMD_READONLY|CMD_FAST,ACL_CATEGORY_KEYSPACE,{{NULL,CMD_KEY_RO|CMD_KEY_ACCESS,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}}},.args=PTTL_Args},
|
||||
{"randomkey","Return a random key from the keyspace","O(1)","1.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_GENERIC,RANDOMKEY_History,RANDOMKEY_tips,randomkeyCommand,1,CMD_READONLY,ACL_CATEGORY_KEYSPACE},
|
||||
{"randomkey","Return a random key from the keyspace","O(1)","1.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_GENERIC,RANDOMKEY_History,RANDOMKEY_tips,randomkeyCommand,1,CMD_READONLY|CMD_TOUCHES_ARBITRARY_KEYS,ACL_CATEGORY_KEYSPACE},
|
||||
{"rename","Rename a key","O(1)","1.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_GENERIC,RENAME_History,RENAME_tips,renameCommand,3,CMD_WRITE,ACL_CATEGORY_KEYSPACE,{{NULL,CMD_KEY_RW|CMD_KEY_ACCESS|CMD_KEY_DELETE,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}},{NULL,CMD_KEY_OW|CMD_KEY_UPDATE,KSPEC_BS_INDEX,.bs.index={2},KSPEC_FK_RANGE,.fk.range={0,1,0}}},.args=RENAME_Args},
|
||||
{"renamenx","Rename a key, only if the new key does not exist","O(1)","1.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_GENERIC,RENAMENX_History,RENAMENX_tips,renamenxCommand,3,CMD_WRITE|CMD_FAST,ACL_CATEGORY_KEYSPACE,{{NULL,CMD_KEY_RW|CMD_KEY_ACCESS|CMD_KEY_DELETE,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}},{NULL,CMD_KEY_OW|CMD_KEY_INSERT,KSPEC_BS_INDEX,.bs.index={2},KSPEC_FK_RANGE,.fk.range={0,1,0}}},.args=RENAMENX_Args},
|
||||
{"restore","Create a key using the provided serialized value, previously obtained using DUMP.","O(1) to create the new key and additional O(N*M) to reconstruct the serialized value, where N is the number of Redis objects composing the value and M their average size. For small string values the time complexity is thus O(1)+O(1*M) where M is small, so simply O(1). However for sorted set values the complexity is O(N*M*log(N)) because inserting values into sorted sets is O(log(N)).","2.6.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_GENERIC,RESTORE_History,RESTORE_tips,restoreCommand,-4,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_KEYSPACE|ACL_CATEGORY_DANGEROUS,{{NULL,CMD_KEY_OW|CMD_KEY_UPDATE,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}}},.args=RESTORE_Args},
|
||||
{"scan","Incrementally iterate the keys space","O(1) for every call. O(N) for a complete iteration, including enough command calls for the cursor to return back to 0. N is the number of elements inside the collection.","2.8.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_GENERIC,SCAN_History,SCAN_tips,scanCommand,-2,CMD_READONLY,ACL_CATEGORY_KEYSPACE,.args=SCAN_Args},
|
||||
{"scan","Incrementally iterate the keys space","O(1) for every call. O(N) for a complete iteration, including enough command calls for the cursor to return back to 0. N is the number of elements inside the collection.","2.8.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_GENERIC,SCAN_History,SCAN_tips,scanCommand,-2,CMD_READONLY|CMD_TOUCHES_ARBITRARY_KEYS,ACL_CATEGORY_KEYSPACE,.args=SCAN_Args},
|
||||
{"sort","Sort the elements in a list, set or sorted set","O(N+M*log(M)) where N is the number of elements in the list or set to sort, and M the number of returned elements. When the elements are not sorted, complexity is O(N).","1.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_GENERIC,SORT_History,SORT_tips,sortCommand,-2,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_SET|ACL_CATEGORY_SORTEDSET|ACL_CATEGORY_LIST|ACL_CATEGORY_DANGEROUS,{{NULL,CMD_KEY_RO|CMD_KEY_ACCESS,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}},{"For the optional BY/GET keyword. It is marked 'unknown' because the key names derive from the content of the key we sort",CMD_KEY_RO|CMD_KEY_ACCESS,KSPEC_BS_UNKNOWN,{{0}},KSPEC_FK_UNKNOWN,{{0}}},{"For the optional STORE keyword. It is marked 'unknown' because the keyword can appear anywhere in the argument array",CMD_KEY_OW|CMD_KEY_UPDATE,KSPEC_BS_UNKNOWN,{{0}},KSPEC_FK_UNKNOWN,{{0}}}},sortGetKeys,.args=SORT_Args},
|
||||
{"sort_ro","Sort the elements in a list, set or sorted set. Read-only variant of SORT.","O(N+M*log(M)) where N is the number of elements in the list or set to sort, and M the number of returned elements. When the elements are not sorted, complexity is O(N).","7.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_GENERIC,SORT_RO_History,SORT_RO_tips,sortroCommand,-2,CMD_READONLY,ACL_CATEGORY_SET|ACL_CATEGORY_SORTEDSET|ACL_CATEGORY_LIST|ACL_CATEGORY_DANGEROUS,{{NULL,CMD_KEY_RO|CMD_KEY_ACCESS,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}},{"For the optional BY/GET keyword. It is marked 'unknown' because the key names derive from the content of the key we sort",CMD_KEY_RO|CMD_KEY_ACCESS,KSPEC_BS_UNKNOWN,{{0}},KSPEC_FK_UNKNOWN,{{0}}}},sortROGetKeys,.args=SORT_RO_Args},
|
||||
{"touch","Alters the last access time of a key(s). Returns the number of existing keys specified.","O(N) where N is the number of keys that will be touched.","3.2.1",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_GENERIC,TOUCH_History,TOUCH_tips,touchCommand,-2,CMD_READONLY|CMD_FAST,ACL_CATEGORY_KEYSPACE,{{NULL,CMD_KEY_RO,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={-1,1,0}}},.args=TOUCH_Args},
|
||||
|
||||
@ -7,7 +7,8 @@
|
||||
"arity": 1,
|
||||
"function": "randomkeyCommand",
|
||||
"command_flags": [
|
||||
"READONLY"
|
||||
"READONLY",
|
||||
"TOUCHES_ARBITRARY_KEYS"
|
||||
],
|
||||
"acl_categories": [
|
||||
"KEYSPACE"
|
||||
|
||||
@ -13,7 +13,8 @@
|
||||
]
|
||||
],
|
||||
"command_flags": [
|
||||
"READONLY"
|
||||
"READONLY",
|
||||
"TOUCHES_ARBITRARY_KEYS"
|
||||
],
|
||||
"acl_categories": [
|
||||
"KEYSPACE"
|
||||
|
||||
4
src/db.c
4
src/db.c
@ -1622,8 +1622,8 @@ void deleteExpiredKeyAndPropagate(redisDb *db, robj *keyobj) {
|
||||
* because call() handles server.also_propagate(); or
|
||||
* 2. Outside of call(): Example: Active-expire, eviction.
|
||||
* In this the caller must remember to call
|
||||
* propagatePendingCommands, preferably at the end of
|
||||
* the deletion batch, so that DELs will be wrapped
|
||||
* postExecutionUnitOperations, preferably just after a
|
||||
* single deletion batch, so that DELs will NOT be wrapped
|
||||
* in MULTI/EXEC */
|
||||
void propagateDeletion(redisDb *db, robj *key, int lazy) {
|
||||
robj *argv[2];
|
||||
|
||||
29
src/server.c
29
src/server.c
@ -3304,29 +3304,35 @@ static void propagatePendingCommands() {
|
||||
|
||||
int j;
|
||||
redisOp *rop;
|
||||
int multi_emitted = 0;
|
||||
|
||||
/* Wrap the commands in server.also_propagate array,
|
||||
* but don't wrap it if we are already in MULTI context,
|
||||
* in case the nested MULTI/EXEC.
|
||||
*
|
||||
* And if the array contains only one command, no need to
|
||||
* wrap it, since the single command is atomic. */
|
||||
if (server.also_propagate.numops > 1) {
|
||||
/* If we got here it means we have finished an execution-unit.
|
||||
* If that unit has caused propagation of multiple commands, they
|
||||
* should be propagated as a transaction */
|
||||
int transaction = server.also_propagate.numops > 1;
|
||||
|
||||
/* In case a command that may modify random keys was run *directly*
|
||||
* (i.e. not from within a script, MULTI/EXEC, RM_Call, etc.) we want
|
||||
* to avoid using a transaction (much like active-expire) */
|
||||
if (server.current_client &&
|
||||
server.current_client->cmd &&
|
||||
server.current_client->cmd->flags & CMD_TOUCHES_ARBITRARY_KEYS)
|
||||
{
|
||||
transaction = 0;
|
||||
}
|
||||
|
||||
if (transaction) {
|
||||
/* We use dbid=-1 to indicate we do not want to replicate SELECT.
|
||||
* It'll be inserted together with the next command (inside the MULTI) */
|
||||
propagateNow(-1,&shared.multi,1,PROPAGATE_AOF|PROPAGATE_REPL);
|
||||
multi_emitted = 1;
|
||||
}
|
||||
|
||||
|
||||
for (j = 0; j < server.also_propagate.numops; j++) {
|
||||
rop = &server.also_propagate.ops[j];
|
||||
serverAssert(rop->target);
|
||||
propagateNow(rop->dbid,rop->argv,rop->argc,rop->target);
|
||||
}
|
||||
|
||||
if (multi_emitted) {
|
||||
if (transaction) {
|
||||
/* We use dbid=-1 to indicate we do not want to replicate select */
|
||||
propagateNow(-1,&shared.exec,1,PROPAGATE_AOF|PROPAGATE_REPL);
|
||||
}
|
||||
@ -4515,6 +4521,7 @@ void addReplyFlagsForCommand(client *c, struct redisCommand *cmd) {
|
||||
{CMD_NO_MULTI, "no_multi"},
|
||||
{CMD_MOVABLE_KEYS, "movablekeys"},
|
||||
{CMD_ALLOW_BUSY, "allow_busy"},
|
||||
/* {CMD_TOUCHES_ARBITRARY_KEYS, "TOUCHES_ARBITRARY_KEYS"}, Hidden on purpose */
|
||||
{0,NULL}
|
||||
};
|
||||
addReplyCommandFlags(c, cmd->flags, flagNames);
|
||||
|
||||
@ -227,6 +227,7 @@ extern int configOOMScoreAdjValuesDefaults[CONFIG_OOM_COUNT];
|
||||
* Populated by populateCommandLegacyRangeSpec. */
|
||||
#define CMD_ALLOW_BUSY ((1ULL<<26))
|
||||
#define CMD_MODULE_GETCHANNELS (1ULL<<27) /* Use the modules getchannels interface. */
|
||||
#define CMD_TOUCHES_ARBITRARY_KEYS (1ULL<<28)
|
||||
|
||||
/* Command flags that describe ACLs categories. */
|
||||
#define ACL_CATEGORY_KEYSPACE (1ULL<<0)
|
||||
@ -2225,6 +2226,12 @@ typedef int redisGetKeysProc(struct redisCommand *cmd, robj **argv, int argc, ge
|
||||
*
|
||||
* CMD_NO_MULTI: The command is not allowed inside a transaction
|
||||
*
|
||||
* CMD_ALLOW_BUSY: The command can run while another command is running for
|
||||
* a long time (timedout script, module command that yields)
|
||||
*
|
||||
* CMD_TOUCHES_ARBITRARY_KEYS: The command may touch (and cause lazy-expire)
|
||||
* arbitrary key (i.e not provided in argv)
|
||||
*
|
||||
* The following additional flags are only used in order to put commands
|
||||
* in a specific ACL category. Commands can have multiple ACL categories.
|
||||
* See redis.conf for the exact meaning of each.
|
||||
|
||||
@ -768,4 +768,46 @@ start_server {tags {"expire"}} {
|
||||
close_replication_stream $repl
|
||||
assert_equal [r debug set-active-expire 1] {OK}
|
||||
} {} {needs:debug}
|
||||
|
||||
test {SCAN: Lazy-expire should not be wrapped in MULTI/EXEC} {
|
||||
r debug set-active-expire 0
|
||||
r flushall
|
||||
|
||||
r set foo1 bar PX 1
|
||||
r set foo2 bar PX 1
|
||||
after 2
|
||||
|
||||
set repl [attach_to_replication_stream]
|
||||
|
||||
r scan 0
|
||||
|
||||
assert_replication_stream $repl {
|
||||
{select *}
|
||||
{del foo*}
|
||||
{del foo*}
|
||||
}
|
||||
close_replication_stream $repl
|
||||
assert_equal [r debug set-active-expire 1] {OK}
|
||||
} {} {needs:debug}
|
||||
|
||||
test {RANDOMKEY: Lazy-expire should not be wrapped in MULTI/EXEC} {
|
||||
r debug set-active-expire 0
|
||||
r flushall
|
||||
|
||||
r set foo1 bar PX 1
|
||||
r set foo2 bar PX 1
|
||||
after 2
|
||||
|
||||
set repl [attach_to_replication_stream]
|
||||
|
||||
r randomkey
|
||||
|
||||
assert_replication_stream $repl {
|
||||
{select *}
|
||||
{del foo*}
|
||||
{del foo*}
|
||||
}
|
||||
close_replication_stream $repl
|
||||
assert_equal [r debug set-active-expire 1] {OK}
|
||||
} {} {needs:debug}
|
||||
}
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user