Redis [QUESTION]About executing unwatch in a multi block

We allow execution of unwatch commands in a transaction blocks.

    /* Exec the command */
    if (c->flags & CLIENT_MULTI &&
        c->cmd->proc != execCommand && c->cmd->proc != discardCommand &&
        c->cmd->proc != multiCommand && c->cmd->proc != watchCommand &&
        c->cmd->proc != resetCommand)
    {
        queueMultiCommand(c);
        addReply(c,shared.queued);
    } else {
        call(c,CMD_CALL_FULL);
        c->woff = server.master_repl_offset;
        if (listLength(server.ready_keys))
            handleClientsBlockedOnKeys();
    }

So the unwatch will be QUEUED

127.0.0.1:6379> watch key1
OK
127.0.0.1:6379> set key1 111
OK
127.0.0.1:6379> multi
OK
127.0.0.1:6379(TX)> unwatch
QUEUED
127.0.0.1:6379(TX)> get key1
QUEUED
127.0.0.1:6379(TX)> exec
(nil)

But absolutely didn't do anything.

void unwatchCommand(client *c) {
    unwatchAllKeys(c);
    c->flags &= (~CLIENT_DIRTY_CAS);
    addReply(c,shared.ok);
}

1: If the key changed, the transaction will fail at the very beginning. Because we check the (CLIENT_DIRTY_CAS|CLIENT_DIRTY_EXEC)) at the very beginning.

    if (c->flags & (CLIENT_DIRTY_CAS|CLIENT_DIRTY_EXEC)) {
        addReply(c, c->flags & CLIENT_DIRTY_EXEC ? shared.execaborterr :
                                                   shared.nullarray[c->resp]);
        discardTransaction(c);
        return;
    }
    ...
    unwatchAllKeys(c); /* Unwatch ASAP otherwise we'll waste CPU cycles */

2: If the key does not change, then the command itself has no effect Because after the flag check, we unwatchAllKeys

So I think it might be better for unwatch to be consistent with watch. (not allowed inside multi, maybe a breaking change..)

void watchCommand(client *c) {
    int j;

    if (c->flags & CLIENT_MULTI) {
        addReplyError(c,"WATCH inside MULTI is not allowed");
        return;
    }
    for (j = 1; j < c->argc; j++)
        watchForKey(c,c->argv[j]);
    addReply(c,shared.ok);
}

About symmetric, i also make a pr about readwriteCommand and readonlyCommand . See https://github.com/redis/redis/pull/9015

Comment From: oranagra

as far as i can tell, there's no reason whatsoever for someone to use UNWATCH after sending MULTI, so in theory we can choose to queue it, or respond with an error. maybe there's some vague use case for actually executing it right away, so that future changes to that key won't fail EXEC, but i'm not sure many clients can make use of that feature (they may already be in pipeline queuing mode, not sending anything to redis).

anyway, changing anything here could be a breaking change so should be done with caution. @enjoy-binbin please look into the history of this, see if there was ever a time were it was handled differently and what were the reasons it was changed.

Comment From: enjoy-binbin

I did not find any more useful information...

I just saw that unwatch can be used in a multi block, and I was a little curious about how to do it. So I went a little deeper, and also want to hear other people's opinions. It's not important. It might be better to keep it as it is

A ref link (Salvatore comment): https://github.com/redis/redis/issues/1315#issue-20859040 In actual scenes, I have always used lua. Sometime use pipeline with multi

Found another one: https://github.com/redis/redis/pull/5609 soloestoy's old pr...

Ready to close this issue at the end of the month