gvsafronov/futriix
1
0
Fork 0
Code Issues Pull Requests Actions 3 Packages Projects Releases Wiki Activity

Fixed SET and BITFIELD commands being wrongly marked movablekeys (#10837)

Browse Source 
The SET and BITFIELD command were added `get_keys_function` in #10148, causing
them to be wrongly marked movablekeys in `populateCommandMovableKeys`.

This was an unintended side effect introduced in #10148 (7.0 RC1)
which could cause some clients an extra round trip for these commands in cluster mode.

Since we define movablekeys as a way to determine if the legacy range [first, last, step]
doesn't find all keys, then we need a completely different approach.

The right approach should be to check if the legacy range covers all key-specs,
and if none of the key-specs have the INCOMPLETE flag. 
This way, we don't need to look at getkeys_proc of VARIABLE_FLAG at all.
Probably with the exception of modules, who may still not be using key-specs.

In this PR, we removed `populateCommandMovableKeys` and put its logic in
`populateCommandLegacyRangeSpec`.
In order to properly serve both old and new modules, we must probably keep relying
CMD_MODULE_GETKEYS, but do that only for modules that don't declare key-specs. 
For ones that do, we need to take the same approach we take with native redis commands.

This approach was proposed by Oran. Fixes #10833

Co-authored-by: Oran Agra <oran@redislabs.com>
This commit is contained in:
Binbin 2022-06-12 13:22:18 +08:00 committed by GitHub
parent 62ac1ab007
commit 92fb4f4f61
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 130 additions and 48 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
src/module.c
View File

@ -1154,15 +1154,10 @@ RedisModuleCommand *moduleCreateCommandProxy(struct RedisModule *module, sds dec
cp->rediscmd->key_specs[0].fk.range.lastkey = lastkey < 0 ? lastkey : (lastkey-firstkey);
cp->rediscmd->key_specs[0].fk.range.keystep = keystep;
cp->rediscmd->key_specs[0].fk.range.limit = 0;
/* Copy the default range to legacy_range_key_spec */
cp->rediscmd->legacy_range_key_spec = cp->rediscmd->key_specs[0];
} else {
cp->rediscmd->key_specs_num = 0;
cp->rediscmd->legacy_range_key_spec.begin_search_type = KSPEC_BS_INVALID;
cp->rediscmd->legacy_range_key_spec.find_keys_type = KSPEC_FK_INVALID;
}
populateCommandMovableKeys(cp->rediscmd);
populateCommandLegacyRangeSpec(cp->rediscmd);
cp->rediscmd->microseconds = 0;
cp->rediscmd->calls = 0;
cp->rediscmd->rejected_calls = 0;
@ -1697,10 +1692,9 @@ int RM_SetCommandInfo(RedisModuleCommand *command, const RedisModuleCommandInfo
}
}
/* Update the legacy (first,last,step) spec used by the COMMAND command,
/* Update the legacy (first,last,step) spec and "movablekeys" flag used by the COMMAND command,
* by trying to "glue" consecutive range key specs. */
populateCommandLegacyRangeSpec(cmd);
populateCommandMovableKeys(cmd);
}
if (info->args) {

77
src/server.c
View File

@ -2643,9 +2643,12 @@ void InitServerLast() {
* By far the most common case is just one range spec (e.g. SET)
* but some commands' ranges were split into two or more ranges
* in order to have different flags for different keys (e.g. SMOVE,
* first key is "read write", second key is "write").
* first key is "RW ACCESS DELETE", second key is "RW INSERT").
*
* This functions uses very basic heuristics and is "best effort":
* Additionally set the CMD_MOVABLE_KEYS flag for commands that may have key
* names in their arguments, but the legacy range spec doesn't cover all of them.
*
* This function uses very basic heuristics and is "best effort":
* 1. Only commands which have only "range" specs are considered.
* 2. Only range specs with keystep of 1 are considered.
* 3. The order of the range specs must be ascending (i.e.
@ -2667,15 +2670,26 @@ void InitServerLast() {
void populateCommandLegacyRangeSpec(struct redisCommand *c) {
memset(&c->legacy_range_key_spec, 0, sizeof(c->legacy_range_key_spec));
if (c->key_specs_num == 0)
/* Set the movablekeys flag if we have a GETKEYS flag for modules.
* Note that for native redis commands, we always have keyspecs,
* with enough information to rely on for movablekeys. */
if (c->flags & CMD_MODULE_GETKEYS)
c->flags |= CMD_MOVABLE_KEYS;
/* no key-specs, no keys, exit. */
if (c->key_specs_num == 0) {
return;
}
if (c->key_specs_num == 1 &&
c->key_specs[0].begin_search_type == KSPEC_BS_INDEX &&
c->key_specs[0].find_keys_type == KSPEC_FK_RANGE)
{
/* Quick win */
/* Quick win, exactly one range spec. */
c->legacy_range_key_spec = c->key_specs[0];
/* If it has the incomplete flag, set the movablekeys flag on the command. */
if (c->key_specs[0].flags & CMD_KEY_INCOMPLETE)
c->flags |= CMD_MOVABLE_KEYS;
return;
}
@ -2684,11 +2698,23 @@ void populateCommandLegacyRangeSpec(struct redisCommand *c) {
for (int i = 0; i < c->key_specs_num; i++) {
if (c->key_specs[i].begin_search_type != KSPEC_BS_INDEX ||
c->key_specs[i].find_keys_type != KSPEC_FK_RANGE)
{
/* Found an incompatible (non range) spec, skip it, and set the movablekeys flag. */
c->flags |= CMD_MOVABLE_KEYS;
continue;
if (c->key_specs[i].fk.range.keystep != 1)
return;
if (prev_lastkey && prev_lastkey != c->key_specs[i].bs.index.pos-1)
return;
}
if (c->key_specs[i].fk.range.keystep != 1 ||
(prev_lastkey && prev_lastkey != c->key_specs[i].bs.index.pos-1))
{
/* Found a range spec that's not plain (step of 1) or not consecutive to the previous one.
* Skip it, and we set the movablekeys flag. */
c->flags |= CMD_MOVABLE_KEYS;
continue;
}
if (c->key_specs[i].flags & CMD_KEY_INCOMPLETE) {
/* The spec we're using is incomplete, we can use it, but we also have to set the movablekeys flag. */
c->flags |= CMD_MOVABLE_KEYS;
}
firstkey = min(firstkey, c->key_specs[i].bs.index.pos);
/* Get the absolute index for lastkey (in the "range" spec, lastkey is relative to firstkey) */
int lastkey_abs_index = c->key_specs[i].fk.range.lastkey;
@ -2696,10 +2722,14 @@ void populateCommandLegacyRangeSpec(struct redisCommand *c) {
lastkey_abs_index += c->key_specs[i].bs.index.pos;
/* For lastkey we use unsigned comparison to handle negative values correctly */
lastkey = max((unsigned)lastkey, (unsigned)lastkey_abs_index);
prev_lastkey = lastkey;
}
if (firstkey == INT_MAX)
if (firstkey == INT_MAX) {
/* Couldn't find range specs, the legacy range spec will remain empty, and we set the movablekeys flag. */
c->flags |= CMD_MOVABLE_KEYS;
return;
}
serverAssert(firstkey != 0);
serverAssert(lastkey != 0);
@ -2787,11 +2817,9 @@ void populateCommandStructure(struct redisCommand *c) {
c->num_tips++;
c->num_args = populateArgsStructure(c->args);
/* Handle the legacy range spec and the "movablekeys" flag (must be done after populating all key specs). */
populateCommandLegacyRangeSpec(c);
/* Handle the "movablekeys" flag (must be done after populating all key specs). */
populateCommandMovableKeys(c);
/* Assign the ID used for ACL. */
c->id = ACLGetCommandID(c->fullname);
@ -3498,31 +3526,6 @@ void afterCommand(client *c) {
}
}
/* Returns 1 for commands that may have key names in their arguments, but the legacy range
* spec doesn't cover all of them. */
void populateCommandMovableKeys(struct redisCommand *cmd) {
int movablekeys = 0;
if (cmd->getkeys_proc || (cmd->flags & CMD_MODULE_GETKEYS)) {
/* Command with getkeys proc */
movablekeys = 1;
} else {
/* Redis command without getkeys proc, but possibly has
* movable keys because of a keys spec. */
for (int i = 0; i < cmd->key_specs_num; i++) {
if (cmd->key_specs[i].begin_search_type != KSPEC_BS_INDEX ||
cmd->key_specs[i].find_keys_type != KSPEC_FK_RANGE)
{
/* If we have a non-range spec it means we have movable keys */
movablekeys = 1;
break;
}
}
}
if (movablekeys)
cmd->flags |= CMD_MOVABLE_KEYS;
}
/* Check if c->cmd exists, fills `err` with details in case it doesn't.
* Return 1 if exists. */
int commandCheckExistence(client *c, sds *err) {

4
src/server.h
View File

@ -215,7 +215,8 @@ extern int configOOMScoreAdjValuesDefaults[CONFIG_OOM_COUNT];
#define CMD_MODULE_NO_CLUSTER (1ULL<<22) /* Deny on Redis Cluster. */
#define CMD_NO_ASYNC_LOADING (1ULL<<23)
#define CMD_NO_MULTI (1ULL<<24)
#define CMD_MOVABLE_KEYS (1ULL<<25) /* populated by populateCommandMovableKeys */
#define CMD_MOVABLE_KEYS (1ULL<<25) /* The legacy range spec doesn't cover all keys.
* Populated by populateCommandLegacyRangeSpec. */
#define CMD_ALLOW_BUSY ((1ULL<<26))
#define CMD_MODULE_GETCHANNELS (1ULL<<27) /* Use the modules getchannels interface. */
@ -3550,7 +3551,6 @@ void mixDigest(unsigned char *digest, const void *ptr, size_t len);
void xorDigest(unsigned char *digest, const void *ptr, size_t len);
sds catSubCommandFullname(const char *parent_name, const char *sub_name);
void commandAddSubcommand(struct redisCommand *parent, struct redisCommand *subcommand, const char *declared_name);
void populateCommandMovableKeys(struct redisCommand *cmd);
void debugDelay(int usec);
void killIOThreads(void);
void killThreads(void);

42
tests/modules/keyspecs.c
View File

@ -18,6 +18,13 @@ int createKspecNone(RedisModuleCtx *ctx) {
return REDISMODULE_OK;
}
int createKspecNoneWithGetkeys(RedisModuleCtx *ctx) {
/* A command without keyspecs; only the legacy (first,last,step) triple (MSET like spec), but also has a getkeys callback */
if (RedisModule_CreateCommand(ctx,"kspec.nonewithgetkeys",kspec_impl,"getkeys-api",1,-1,2) == REDISMODULE_ERR)
return REDISMODULE_ERR;
return REDISMODULE_OK;
}
int createKspecTwoRanges(RedisModuleCtx *ctx) {
/* Test that two position/range-based key specs are combined to produce the
* legacy (first,last,step) values representing both keys. */
@ -51,6 +58,39 @@ int createKspecTwoRanges(RedisModuleCtx *ctx) {
return REDISMODULE_OK;
}
int createKspecTwoRangesWithGap(RedisModuleCtx *ctx) {
/* Test that two position/range-based key specs are combined to produce the
* legacy (first,last,step) values representing just one key. */
if (RedisModule_CreateCommand(ctx,"kspec.tworangeswithgap",kspec_impl,"",0,0,0) == REDISMODULE_ERR)
return REDISMODULE_ERR;
RedisModuleCommand *command = RedisModule_GetCommand(ctx,"kspec.tworangeswithgap");
RedisModuleCommandInfo info = {
.version = REDISMODULE_COMMAND_INFO_VERSION,
.arity = -2,
.key_specs = (RedisModuleCommandKeySpec[]){
{
.flags = REDISMODULE_CMD_KEY_RO | REDISMODULE_CMD_KEY_ACCESS,
.begin_search_type = REDISMODULE_KSPEC_BS_INDEX,
.bs.index.pos = 1,
.find_keys_type = REDISMODULE_KSPEC_FK_RANGE,
.fk.range = {0,1,0}
},
{
.flags = REDISMODULE_CMD_KEY_RW | REDISMODULE_CMD_KEY_UPDATE,
.begin_search_type = REDISMODULE_KSPEC_BS_INDEX,
.bs.index.pos = 3,
/* Omitted find_keys_type is shorthand for RANGE {0,1,0} */
},
{0}
}
};
if (RedisModule_SetCommandInfo(command, &info) == REDISMODULE_ERR)
return REDISMODULE_ERR;
return REDISMODULE_OK;
}
int createKspecKeyword(RedisModuleCtx *ctx) {
/* Only keyword-based specs. The legacy triple is wiped and set to (0,0,0). */
if (RedisModule_CreateCommand(ctx,"kspec.keyword",kspec_impl,"",3,-1,1) == REDISMODULE_ERR)
@ -177,7 +217,9 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc)
return REDISMODULE_ERR;
if (createKspecNone(ctx) == REDISMODULE_ERR) return REDISMODULE_ERR;
if (createKspecNoneWithGetkeys(ctx) == REDISMODULE_ERR) return REDISMODULE_ERR;
if (createKspecTwoRanges(ctx) == REDISMODULE_ERR) return REDISMODULE_ERR;
if (createKspecTwoRangesWithGap(ctx) == REDISMODULE_ERR) return REDISMODULE_ERR;
if (createKspecKeyword(ctx) == REDISMODULE_ERR) return REDISMODULE_ERR;
if (createKspecComplex1(ctx) == REDISMODULE_ERR) return REDISMODULE_ERR;
if (createKspecComplex2(ctx) == REDISMODULE_ERR) return REDISMODULE_ERR;

15
tests/unit/introspection-2.tcl
View File

@ -176,4 +176,19 @@ start_server {tags {"introspection"}} {
assert_equal {{}} [r command info get|key]
assert_equal {{}} [r command info config|get|key]
}
foreach cmd {SET GET MSET BITFIELD LMOVE LPOP BLPOP PING MEMORY MEMORY|USAGE RENAME GEORADIUS_RO} {
test "$cmd command will not be marked with movablekeys" {
set info [lindex [r command info $cmd] 0]
assert_no_match {*movablekeys*} [lindex $info 2]
}
}
foreach cmd {ZUNIONSTORE XREAD EVAL SORT SORT_RO MIGRATE GEORADIUS} {
test "$cmd command is marked with movablekeys" {
set info [lindex [r command info $cmd] 0]
assert_match {*movablekeys*} [lindex $info 2]
}
}
}

30
tests/unit/moduleapi/keyspecs.tcl
View File

@ -31,6 +31,20 @@ start_server {tags {"modules"}} {
assert_equal [r command getkeys kspec.tworanges foo bar baz quux] {foo bar}
}
test "Module key specs: Two ranges with gap" {
set reply [lindex [r command info kspec.tworangeswithgap] 0]
# Verify (first, last, step) and movablekeys
assert_equal [lindex $reply 2] {module movablekeys}
assert_equal [lindex $reply 3] 1
assert_equal [lindex $reply 4] 1
assert_equal [lindex $reply 5] 1
# Verify key-specs
set keyspecs [lindex $reply 8]
assert_equal [lindex $keyspecs 0] {flags {RO access} begin_search {type index spec {index 1}} find_keys {type range spec {lastkey 0 keystep 1 limit 0}}}
assert_equal [lindex $keyspecs 1] {flags {RW update} begin_search {type index spec {index 3}} find_keys {type range spec {lastkey 0 keystep 1 limit 0}}}
assert_equal [r command getkeys kspec.tworangeswithgap foo bar baz quux] {foo baz}
}
test "Module key specs: Keyword-only spec clears the legacy triple" {
set reply [lindex [r command info kspec.keyword] 0]
# Verify (first, last, step) and movablekeys
@ -79,7 +93,7 @@ start_server {tags {"modules"}} {
test "Module command list filtering" {
;# Note: we piggyback this tcl file to test the general functionality of command list filtering
set reply [r command list filterby module keyspecs]
assert_equal [lsort $reply] {kspec.complex1 kspec.complex2 kspec.keyword kspec.none kspec.tworanges}
assert_equal [lsort $reply] {kspec.complex1 kspec.complex2 kspec.keyword kspec.none kspec.nonewithgetkeys kspec.tworanges kspec.tworangeswithgap}
assert_equal [r command getkeys kspec.complex2 foo bar 2 baz quux banana STORE dst dummy MOREKEYS hey ho] {dst foo bar baz quux hey ho}
}
@ -108,6 +122,20 @@ start_server {tags {"modules"}} {
assert_equal "This user has no permissions to access the 'write' key" [r ACL DRYRUN testuser kspec.tworanges write rw]
}
foreach cmd {kspec.none kspec.tworanges} {
test "$cmd command will not be marked with movablekeys" {
set info [lindex [r command info $cmd] 0]
assert_no_match {*movablekeys*} [lindex $info 2]
}
}
foreach cmd {kspec.keyword kspec.complex1 kspec.complex2 kspec.nonewithgetkeys} {
test "$cmd command is marked with movablekeys" {
set info [lindex [r command info $cmd] 0]
assert_match {*movablekeys*} [lindex $info 2]
}
}
test "Unload the module - keyspecs" {
assert_equal {OK} [r module unload keyspecs]
}