starting from https://github.com/redis/redis/pull/9504 not all commands are in server.commands (that is, if we are counting subcommands as commands, which we do)

we need to go over all references to server.commands and server.orig_commands and make sure that the usage is as intended (i.e. if the functions need to process only "top level" commands, and ignore subcommands. i assume this is not the case for most usages, but rather just to iterate all commands, in which case we will need to replace the logic with some recursive function tat takes. redisCommand - see populateCommandStructure)

Comment From: guybe7

known issues: 1. COMMAND LIST is not iterating subcommand 2. ACL CAT is not iterating subcommand 3. moduleUnregisterCommands doesn't dive into subcommand, causing a memory leak

Comment From: enjoy-binbin

added in #10127 take care of 1 / 2 / 3, i am checking others references @guybe7 PTAL with the module part

@oranagra btw, should we add this in 10127?

Comment From: oranagra

i haven't reviewed the commit in detail, but yes, i think both topics can be handled in the same PR

Comment From: enjoy-binbin

i think what we left are: 4. COMMAND (no args) is not iterating subcommand, no need, right? 5. COMMAND COUNT is not counting subcommand, no need, right? 6. config rename-command, should we support rename a subcommand?

should i do the cast? i see other places do this.

struct redisCommand *sub = (struct redisCommand *)dictGetVal(de);

Comment From: oranagra

  • i think COMMAND (no args), and COMMAND INFO (no args) are already doing what they should, which is add the sub-commands as a nested reply.
  • which is why i think COMMAND COUNT should not be changed too.

I'm not sure about rename-command.. it's a deprecated feature, so i don't think it should support sub-commands. all we need to make sure is that when renaming a parent command, the sub-commands are renamed too. i.e. people usually used it to prevent access to FLUSHDB or DEBUG. so we need to make sure that if CLIENT is renamed, then CLIENT KILL become inaccessible (as it was in previous versions)

Comment From: enjoy-binbin

yes, like i rename client to cli (current unstable branch)

127.0.0.1:6379> client list
ERR unknown command 'client', with args beginning with: 'list' 
127.0.0.1:6379> cli list
"id=3 addr=127.0.0.1:47696 laddr=127.0.0.1:6379 fd=8 name= age=23 idle=0 flags=N db=0 sub=0 psub=0 multi=-1 qbuf=23 qbuf-free=20451 argv-mem=7 multi-mem=0 obl=0 oll=0 omem=0 tot-mem=40983 events=r cmd=client|list user=default redir=-1 resp=2\n"
127.0.0.1:6379> cli list2
(error) ERR Unknown subcommand 'list2'. Try CLI HELP.

Comment From: oranagra

that's sufficient IMHO