From 7a3cc8cc32f0723ebe24c0d4340db864030f9b5a Mon Sep 17 00:00:00 2001 From: Twice Date: Sun, 3 Nov 2024 00:50:08 +0800 Subject: [PATCH] feat(cmd): add blocking flag and remove useless flags (#2637) Co-authored-by: hulk --- src/commands/cmd_function.cc | 9 +++++---- src/commands/cmd_list.cc | 9 +++++---- src/commands/cmd_pubsub.cc | 19 +++++++++---------- src/commands/cmd_replication.cc | 11 +++++------ src/commands/cmd_script.cc | 12 ++++++------ src/commands/cmd_server.cc | 2 +- src/commands/cmd_stream.cc | 4 ++-- src/commands/cmd_zset.cc | 6 +++--- src/commands/commander.h | 17 +++++------------ src/server/redis_connection.cc | 11 +++++++---- 10 files changed, 48 insertions(+), 52 deletions(-) diff --git a/src/commands/cmd_function.cc b/src/commands/cmd_function.cc index ff41257a488..c229cdaa24e 100644 --- a/src/commands/cmd_function.cc +++ b/src/commands/cmd_function.cc @@ -109,9 +109,10 @@ uint64_t GenerateFunctionFlags(uint64_t flags, const std::vector &a return flags; } -REDIS_REGISTER_COMMANDS( - Function, MakeCmdAttr("function", -2, "exclusive no-script", NO_KEY, GenerateFunctionFlags), - MakeCmdAttr>("fcall", -3, "exclusive write no-script", GetScriptEvalKeyRange), - MakeCmdAttr>("fcall_ro", -3, "read-only ro-script no-script", GetScriptEvalKeyRange)); +REDIS_REGISTER_COMMANDS(Function, + MakeCmdAttr("function", -2, "exclusive no-script", NO_KEY, + GenerateFunctionFlags), + MakeCmdAttr>("fcall", -3, "exclusive write no-script", GetScriptEvalKeyRange), + MakeCmdAttr>("fcall_ro", -3, "read-only no-script", GetScriptEvalKeyRange)); } // namespace redis diff --git a/src/commands/cmd_list.cc b/src/commands/cmd_list.cc index 367d8db6443..cf588ec0891 100644 --- a/src/commands/cmd_list.cc +++ b/src/commands/cmd_list.cc @@ -865,14 +865,15 @@ class CommandLPos : public Commander { PosSpec spec_; }; -REDIS_REGISTER_COMMANDS(List, MakeCmdAttr("blpop", -3, "write no-script", 1, -2, 1), - MakeCmdAttr("brpop", -3, "write no-script", 1, -2, 1), - MakeCmdAttr("blmpop", -5, "write no-script", CommandBLMPop::keyRangeGen), +REDIS_REGISTER_COMMANDS(List, MakeCmdAttr("blpop", -3, "write no-script blocking", 1, -2, 1), + MakeCmdAttr("brpop", -3, "write no-script blocking", 1, -2, 1), + MakeCmdAttr("blmpop", -5, "write no-script blocking", + CommandBLMPop::keyRangeGen), MakeCmdAttr("lindex", 3, "read-only", 1, 1, 1), MakeCmdAttr("linsert", 5, "write slow", 1, 1, 1), MakeCmdAttr("llen", 2, "read-only", 1, 1, 1), MakeCmdAttr("lmove", 5, "write", 1, 2, 1), - MakeCmdAttr("blmove", 6, "write", 1, 2, 1), + MakeCmdAttr("blmove", 6, "write blocking", 1, 2, 1), MakeCmdAttr("lpop", -2, "write", 1, 1, 1), // MakeCmdAttr("lpos", -3, "read-only", 1, 1, 1), MakeCmdAttr("lpush", -3, "write", 1, 1, 1), diff --git a/src/commands/cmd_pubsub.cc b/src/commands/cmd_pubsub.cc index e7e4dee7826..8aaa7f4bab8 100644 --- a/src/commands/cmd_pubsub.cc +++ b/src/commands/cmd_pubsub.cc @@ -252,15 +252,14 @@ class CommandPubSub : public Commander { std::string subcommand_; }; -REDIS_REGISTER_COMMANDS( - Pubsub, MakeCmdAttr("publish", 3, "read-only pub-sub", NO_KEY), - MakeCmdAttr("mpublish", -3, "read-only pub-sub", NO_KEY), - MakeCmdAttr("subscribe", -2, "read-only pub-sub no-multi no-script", NO_KEY), - MakeCmdAttr("unsubscribe", -1, "read-only pub-sub no-multi no-script", NO_KEY), - MakeCmdAttr("psubscribe", -2, "read-only pub-sub no-multi no-script", NO_KEY), - MakeCmdAttr("punsubscribe", -1, "read-only pub-sub no-multi no-script", NO_KEY), - MakeCmdAttr("ssubscribe", -2, "read-only pub-sub no-multi no-script", NO_KEY), - MakeCmdAttr("sunsubscribe", -1, "read-only pub-sub no-multi no-script", NO_KEY), - MakeCmdAttr("pubsub", -2, "read-only pub-sub no-script", NO_KEY), ) +REDIS_REGISTER_COMMANDS(Pubsub, MakeCmdAttr("publish", 3, "read-only", NO_KEY), + MakeCmdAttr("mpublish", -3, "read-only", NO_KEY), + MakeCmdAttr("subscribe", -2, "read-only no-multi no-script", NO_KEY), + MakeCmdAttr("unsubscribe", -1, "read-only no-multi no-script", NO_KEY), + MakeCmdAttr("psubscribe", -2, "read-only no-multi no-script", NO_KEY), + MakeCmdAttr("punsubscribe", -1, "read-only no-multi no-script", NO_KEY), + MakeCmdAttr("ssubscribe", -2, "read-only no-multi no-script", NO_KEY), + MakeCmdAttr("sunsubscribe", -1, "read-only no-multi no-script", NO_KEY), + MakeCmdAttr("pubsub", -2, "read-only no-script", NO_KEY), ) } // namespace redis diff --git a/src/commands/cmd_replication.cc b/src/commands/cmd_replication.cc index e649516eef4..4d46e11d9ff 100644 --- a/src/commands/cmd_replication.cc +++ b/src/commands/cmd_replication.cc @@ -344,11 +344,10 @@ class CommandDBName : public Commander { } }; -REDIS_REGISTER_COMMANDS( - Replication, MakeCmdAttr("replconf", -3, "read-only replication no-script", NO_KEY), - MakeCmdAttr("psync", -2, "read-only replication no-multi no-script", NO_KEY), - MakeCmdAttr("_fetch_meta", 1, "read-only replication no-multi no-script", NO_KEY), - MakeCmdAttr("_fetch_file", 2, "read-only replication no-multi no-script", NO_KEY), - MakeCmdAttr("_db_name", 1, "read-only replication no-multi", NO_KEY), ) +REDIS_REGISTER_COMMANDS(Replication, MakeCmdAttr("replconf", -3, "read-only no-script", NO_KEY), + MakeCmdAttr("psync", -2, "read-only no-multi no-script", NO_KEY), + MakeCmdAttr("_fetch_meta", 1, "read-only no-multi no-script", NO_KEY), + MakeCmdAttr("_fetch_file", 2, "read-only no-multi no-script", NO_KEY), + MakeCmdAttr("_db_name", 1, "read-only no-multi", NO_KEY), ) } // namespace redis diff --git a/src/commands/cmd_script.cc b/src/commands/cmd_script.cc index 3098d0058da..b4cf3539f52 100644 --- a/src/commands/cmd_script.cc +++ b/src/commands/cmd_script.cc @@ -125,11 +125,11 @@ uint64_t GenerateScriptFlags(uint64_t flags, const std::vector &arg return flags; } -REDIS_REGISTER_COMMANDS( - Script, MakeCmdAttr("eval", -3, "exclusive write no-script", GetScriptEvalKeyRange), - MakeCmdAttr("evalsha", -3, "exclusive write no-script", GetScriptEvalKeyRange), - MakeCmdAttr("eval_ro", -3, "read-only no-script ro-script", GetScriptEvalKeyRange), - MakeCmdAttr("evalsha_ro", -3, "read-only no-script ro-script", GetScriptEvalKeyRange), - MakeCmdAttr("script", -2, "exclusive no-script", NO_KEY), ) +REDIS_REGISTER_COMMANDS(Script, + MakeCmdAttr("eval", -3, "exclusive write no-script", GetScriptEvalKeyRange), + MakeCmdAttr("evalsha", -3, "exclusive write no-script", GetScriptEvalKeyRange), + MakeCmdAttr("eval_ro", -3, "read-only no-script", GetScriptEvalKeyRange), + MakeCmdAttr("evalsha_ro", -3, "read-only no-script", GetScriptEvalKeyRange), + MakeCmdAttr("script", -2, "exclusive no-script", NO_KEY), ) } // namespace redis diff --git a/src/commands/cmd_server.cc b/src/commands/cmd_server.cc index 16fea925d24..e2938267710 100644 --- a/src/commands/cmd_server.cc +++ b/src/commands/cmd_server.cc @@ -1364,7 +1364,7 @@ REDIS_REGISTER_COMMANDS(Server, MakeCmdAttr("auth", 2, "read-only o MakeCmdAttr("slaveof", 3, "read-only exclusive no-script", NO_KEY), MakeCmdAttr("stats", 1, "read-only", NO_KEY), MakeCmdAttr("rdb", -3, "write exclusive", NO_KEY), - MakeCmdAttr("reset", 1, "ok-loading multi no-script pub-sub", NO_KEY), + MakeCmdAttr("reset", 1, "ok-loading multi no-script", NO_KEY), MakeCmdAttr("applybatch", -2, "write no-multi", NO_KEY), MakeCmdAttr("dump", 2, "read-only", 1, 1, 1), MakeCmdAttr("pollupdates", -2, "read-only", NO_KEY), ) diff --git a/src/commands/cmd_stream.cc b/src/commands/cmd_stream.cc index a7aa98025dc..521e5ca312f 100644 --- a/src/commands/cmd_stream.cc +++ b/src/commands/cmd_stream.cc @@ -1888,8 +1888,8 @@ REDIS_REGISTER_COMMANDS(Stream, MakeCmdAttr("xack", -4, "write no-d MakeCmdAttr("xpending", -3, "read-only", 1, 1, 1), MakeCmdAttr("xrange", -4, "read-only", 1, 1, 1), MakeCmdAttr("xrevrange", -2, "read-only", 1, 1, 1), - MakeCmdAttr("xread", -4, "read-only", NO_KEY), - MakeCmdAttr("xreadgroup", -7, "write", NO_KEY), + MakeCmdAttr("xread", -4, "read-only blocking", NO_KEY), + MakeCmdAttr("xreadgroup", -7, "write blocking", NO_KEY), MakeCmdAttr("xtrim", -4, "write no-dbsize-check", 1, 1, 1), MakeCmdAttr("xsetid", -3, "write", 1, 1, 1)) diff --git a/src/commands/cmd_zset.cc b/src/commands/cmd_zset.cc index d26abb54b91..5b1f392f1a7 100644 --- a/src/commands/cmd_zset.cc +++ b/src/commands/cmd_zset.cc @@ -1549,10 +1549,10 @@ REDIS_REGISTER_COMMANDS(ZSet, MakeCmdAttr("zadd", -4, "write", 1, 1 MakeCmdAttr("zlexcount", 4, "read-only", 1, 1, 1), MakeCmdAttr("zpopmax", -2, "write", 1, 1, 1), MakeCmdAttr("zpopmin", -2, "write", 1, 1, 1), - MakeCmdAttr("bzpopmax", -3, "write", 1, -2, 1), - MakeCmdAttr("bzpopmin", -3, "write", 1, -2, 1), + MakeCmdAttr("bzpopmax", -3, "write blocking", 1, -2, 1), + MakeCmdAttr("bzpopmin", -3, "write blocking", 1, -2, 1), MakeCmdAttr("zmpop", -4, "write", CommandZMPop::Range), - MakeCmdAttr("bzmpop", -5, "write", CommandBZMPop::Range), + MakeCmdAttr("bzmpop", -5, "write blocking", CommandBZMPop::Range), MakeCmdAttr("zrangestore", -5, "write", 1, 1, 1), MakeCmdAttr("zrange", -4, "read-only", 1, 1, 1), MakeCmdAttr("zrevrange", -4, "read-only", 1, 1, 1), diff --git a/src/commands/commander.h b/src/commands/commander.h index 87efdb0131e..066c4ce86d8 100644 --- a/src/commands/commander.h +++ b/src/commands/commander.h @@ -58,18 +58,15 @@ struct CommandAttributes; enum CommandFlags : uint64_t { kCmdWrite = 1ULL << 0, // "write" flag kCmdReadOnly = 1ULL << 1, // "read-only" flag - kCmdReplication = 1ULL << 2, // "replication" flag - kCmdPubSub = 1ULL << 3, // "pub-sub" flag - kCmdScript = 1ULL << 4, // "script" flag kCmdLoading = 1ULL << 5, // "ok-loading" flag - kCmdMulti = 1ULL << 6, // "multi" flag + kCmdEndMulti = 1ULL << 6, // "multi" flag, for ending a MULTI scope kCmdExclusive = 1ULL << 7, // "exclusive" flag kCmdNoMulti = 1ULL << 8, // "no-multi" flag kCmdNoScript = 1ULL << 9, // "no-script" flag - kCmdROScript = 1ULL << 10, // "ro-script" flag for read-only script commands kCmdCluster = 1ULL << 11, // "cluster" flag kCmdNoDBSizeCheck = 1ULL << 12, // "no-dbsize-check" flag kCmdSlow = 1ULL << 13, // "slow" flag + kCmdBlocking = 1ULL << 14, // "blocking" flag }; enum class CommandCategory : uint8_t { @@ -302,28 +299,24 @@ inline uint64_t ParseCommandFlags(const std::string &description, const std::str flags |= kCmdWrite; else if (flag == "read-only") flags |= kCmdReadOnly; - else if (flag == "replication") - flags |= kCmdReplication; - else if (flag == "pub-sub") - flags |= kCmdPubSub; else if (flag == "ok-loading") flags |= kCmdLoading; else if (flag == "exclusive") flags |= kCmdExclusive; else if (flag == "multi") - flags |= kCmdMulti; + flags |= kCmdEndMulti; else if (flag == "no-multi") flags |= kCmdNoMulti; else if (flag == "no-script") flags |= kCmdNoScript; - else if (flag == "ro-script") - flags |= kCmdROScript; else if (flag == "cluster") flags |= kCmdCluster; else if (flag == "no-dbsize-check") flags |= kCmdNoDBSizeCheck; else if (flag == "slow") flags |= kCmdSlow; + else if (flag == "blocking") + flags |= kCmdBlocking; else { std::cout << fmt::format("Encountered non-existent flag '{}' in command {} in command attribute parsing", flag, cmd_name) diff --git a/src/server/redis_connection.cc b/src/server/redis_connection.cc index 24ea47b9051..584b506839a 100644 --- a/src/server/redis_connection.cc +++ b/src/server/redis_connection.cc @@ -425,8 +425,11 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { concurrency = srv_->WorkConcurrencyGuard(); } - if (cmd_flags & kCmdROScript) { - // if executing read only lua script commands, set current connection. + auto category = attributes->category; + if ((category == CommandCategory::Function || category == CommandCategory::Script) && (cmd_flags & kCmdReadOnly)) { + // FIXME: since read-only script commands are not exclusive, + // SetCurrentConnection here is weird and can cause many issues, + // we should pass the Connection directly to the lua context instead srv_->SetCurrentConnection(this); } @@ -471,8 +474,8 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { DisableFlag(kAsking); } - // We don't execute commands, but queue them, ant then execute in EXEC command - if (is_multi_exec && !in_exec_ && !(cmd_flags & kCmdMulti)) { + // We don't execute commands, but queue them, and then execute in EXEC command + if (is_multi_exec && !in_exec_ && !(cmd_flags & kCmdEndMulti)) { multi_cmds_.emplace_back(cmd_tokens); Reply(redis::SimpleString("QUEUED")); continue;