Redis [NEW] Expect to use transaction with all read-only commands on replica node in cluster mode if all keys can be found on replica

The problem/use-case that the feature addresses

In Redis Cluster mode, even if 'readonly' command has already been called on replica, the transaction will not work even if all keys can be found on the replica node.

Description of the feature

As Redis user, I would expect to use transaction with all read-only commands on replica node in cluster mode if all keys can be found on replica.

Additional information I did the following tests on my replica in cluster mode with redis-cli

Without transaction:

readonly
OK
hget hashtest a
"1"
hget hashtest b
"2"

With transaction:

readonly
multi
OK
hget hashtest a
QUEUED
hget hashtest b
QUEUED
exec
(error) MOVED XXXXXXXXX

Comment From: madolson

The specific reason this fails is because exec is not treated as a READONLY command and the redirect flag ends up getting thrown to call the master. I think it's strictly safe to execute the multi-exec if all the commands contained are readonly. We could consider generalizing it more broadly, but I think for now that is sufficient for resolving this use case.

Marking this as a "feature" since it's technically documented that this isn't supported.

Comment From: oranagra

isn't it as simple as doing this (similar to what processCommand does in 6.0.6)?

diff --git a/src/cluster.c b/src/cluster.c
index ba0ada04a..b9a0c8661 100644
--- a/src/cluster.c
+++ b/src/cluster.c
@@ -5769,8 +5769,10 @@ clusterNode *getNodeByQuery(client *c, struct redisCommand *cmd, robj **argv, in
     /* Handle the read-only client case reading from a slave: if this
      * node is a slave and the request is about an hash slot our master
      * is serving, we can reply without redirection. */
+    int is_readonly_command = (c->cmd->flags & CMD_READONLY) ||
+                              (c->cmd->proc == execCommand && !(c->mstate.cmd_inv_flags & CMD_READONLY));
     if (c->flags & CLIENT_READONLY &&
-        (cmd->flags & CMD_READONLY || cmd->proc == evalCommand ||
+        (is_readonly_command || cmd->proc == evalCommand ||
          cmd->proc == evalShaCommand) &&
         nodeIsSlave(myself) &&
         myself->slaveof == n)

@WeiSongAmazon care to test it?

Comment From: WeiSongAmazon

@oranagra Sure, I will follow and test the change.

Comment From: eliblight

CONFIRMED! This suggested change produce the expected behaviour and I have added a TCL test for good measures.

PR is on the way