From 73b4e663e01e3fd926579a1bd9007798d8e2a827 Mon Sep 17 00:00:00 2001 From: John Sully Date: Fri, 28 Jun 2024 16:27:26 +0000 Subject: [PATCH 1/4] When in cluster mode we should be more permissive about what we return with MGET. Currently we check if all keys are the same slot but we could retunr if all slots are on this node instead. Signed-off-by: John Sully --- src/cluster.c | 10 ++++++++-- src/server.c | 3 ++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/cluster.c b/src/cluster.c index caa1e5798b..1b6c737194 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -1080,8 +1080,14 @@ getNodeByQuery(client *c, struct serverCommand *cmd, robj **argv, int argc, int } } else { /* If it is not the first key/channel, make sure it is exactly - * the same key/channel as the first we saw. */ - if (slot != thisslot) { + * the same key/channel as the first we saw or an allowed crossslot situation. */ + int prevent_crossslot = (cmd_flags & CMD_WRITE) // eliminate issues with client->current_slot + || pubsubshard_included // pubsub does not benefit and too many edge cases + || c->cmd->proc == execCommand // We do not permit crossslot transactions to prevent client code which will break when cluster topology changes + // finally, if any key is migrating we cannot permit the crossslot since we don't check if the specific keys are affected + // this could potentially be relaxed in the future. + || migrating_slot || importing_slot || getMigratingSlotDest(slot) != NULL || getImportingSlotSource(slot) != NULL; + if (slot != thisslot && prevent_crossslot) { /* Error: multiple keys from different slots. */ getKeysFreeResult(&result); if (error_code) *error_code = CLUSTER_REDIR_CROSS_SLOT; diff --git a/src/server.c b/src/server.c index 898675ed04..313fea1c71 100644 --- a/src/server.c +++ b/src/server.c @@ -5646,7 +5646,8 @@ sds genValkeyInfoString(dict *section_dict, int all_sections, int everything) { "executable:%s\r\n", server.executable ? server.executable : "", "config_file:%s\r\n", server.configfile ? server.configfile : "", "io_threads_active:%i\r\n", server.active_io_threads_num > 1, - "availability_zone:%s\r\n", server.availability_zone)); + "availability_zone:%s\r\n", server.availability_zone, + "features:%s\r\n", "cluster_mget")); /* Conditional properties */ if (isShutdownInitiated()) { From 9ec45bec8cd6ffef471537b76e40fee71e514721 Mon Sep 17 00:00:00 2001 From: John Sully Date: Fri, 28 Jun 2024 16:36:25 +0000 Subject: [PATCH 2/4] Fix formatting Signed-off-by: John Sully --- src/cluster.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/cluster.c b/src/cluster.c index 1b6c737194..2901a815ef 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -1081,12 +1081,16 @@ getNodeByQuery(client *c, struct serverCommand *cmd, robj **argv, int argc, int } else { /* If it is not the first key/channel, make sure it is exactly * the same key/channel as the first we saw or an allowed crossslot situation. */ - int prevent_crossslot = (cmd_flags & CMD_WRITE) // eliminate issues with client->current_slot - || pubsubshard_included // pubsub does not benefit and too many edge cases - || c->cmd->proc == execCommand // We do not permit crossslot transactions to prevent client code which will break when cluster topology changes - // finally, if any key is migrating we cannot permit the crossslot since we don't check if the specific keys are affected + int prevent_crossslot = + (cmd_flags & CMD_WRITE) // eliminate issues with client->current_slot + || pubsubshard_included // pubsub does not benefit and too many edge cases + || c->cmd->proc == execCommand // We do not permit crossslot transactions to prevent client code + // which will break when cluster topology changes + // finally, if any key is migrating we cannot permit the crossslot since we don't check if the + // specific keys are affected // this could potentially be relaxed in the future. - || migrating_slot || importing_slot || getMigratingSlotDest(slot) != NULL || getImportingSlotSource(slot) != NULL; + || migrating_slot || importing_slot || getMigratingSlotDest(slot) != NULL || + getImportingSlotSource(slot) != NULL; if (slot != thisslot && prevent_crossslot) { /* Error: multiple keys from different slots. */ getKeysFreeResult(&result); From b0dfe05d13b7b4414afeb9355f470ec37081e03b Mon Sep 17 00:00:00 2001 From: John Sully Date: Tue, 28 Jan 2025 20:01:01 +0000 Subject: [PATCH 3/4] Ensure that multi-slot queries work with slot caching --- src/cluster.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/src/cluster.c b/src/cluster.c index 2901a815ef..794c9034a8 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -989,6 +989,7 @@ getNodeByQuery(client *c, struct serverCommand *cmd, robj **argv, int argc, int clusterNode *n = NULL; robj *firstkey = NULL; int multiple_keys = 0; + int multi_slot = false; multiState *ms, _ms; multiCmd mc; int i, slot = 0, migrating_slot = 0, importing_slot = 0, missing_keys = 0, existing_keys = 0; @@ -1089,13 +1090,17 @@ getNodeByQuery(client *c, struct serverCommand *cmd, robj **argv, int argc, int // finally, if any key is migrating we cannot permit the crossslot since we don't check if the // specific keys are affected // this could potentially be relaxed in the future. - || migrating_slot || importing_slot || getMigratingSlotDest(slot) != NULL || - getImportingSlotSource(slot) != NULL; - if (slot != thisslot && prevent_crossslot) { - /* Error: multiple keys from different slots. */ - getKeysFreeResult(&result); - if (error_code) *error_code = CLUSTER_REDIR_CROSS_SLOT; - return NULL; + || migrating_slot || importing_slot || getMigratingSlotDest(thisslot) != NULL || + getImportingSlotSource(thisslot) != NULL || n != getNodeBySlot(thisslot); + if (slot != thisslot) { + if (prevent_crossslot) { + /* Error: multiple keys from different slots. */ + getKeysFreeResult(&result); + if (error_code) *error_code = CLUSTER_REDIR_CROSS_SLOT; + return NULL; + } else { + multi_slot = true; + } } if (importing_slot && !multiple_keys && !equalStringObjects(firstkey, thiskey)) { /* Flag this request as one with multiple different @@ -1150,7 +1155,11 @@ getNodeByQuery(client *c, struct serverCommand *cmd, robj **argv, int argc, int } /* Return the hashslot by reference. */ - if (hashslot) *hashslot = slot; + if (hashslot) { + // If we have multiple slots we disable slot caching on the client + // In the future perhaps this optimization could be extended to work with multiple keys + *hashslot = multi_slot ? -1 : slot; + } /* MIGRATE always works in the context of the local node if the slot * is open (migrating or importing state). We need to be able to freely From 897d36ebf3edef34f429ad2c95c83b377cc26ca7 Mon Sep 17 00:00:00 2001 From: John Sully Date: Tue, 28 Jan 2025 20:17:23 +0000 Subject: [PATCH 4/4] Improve C compatibility --- src/cluster.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cluster.c b/src/cluster.c index 794c9034a8..2f682e5217 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -989,7 +989,7 @@ getNodeByQuery(client *c, struct serverCommand *cmd, robj **argv, int argc, int clusterNode *n = NULL; robj *firstkey = NULL; int multiple_keys = 0; - int multi_slot = false; + int multi_slot = 0; multiState *ms, _ms; multiCmd mc; int i, slot = 0, migrating_slot = 0, importing_slot = 0, missing_keys = 0, existing_keys = 0; @@ -1099,7 +1099,7 @@ getNodeByQuery(client *c, struct serverCommand *cmd, robj **argv, int argc, int if (error_code) *error_code = CLUSTER_REDIR_CROSS_SLOT; return NULL; } else { - multi_slot = true; + multi_slot = 1; } } if (importing_slot && !multiple_keys && !equalStringObjects(firstkey, thiskey)) {