• Oran Agra's avatar
    Remove read-only flag from non-keyspace cmds, different approach for EXEC to... · 411c18bb
    Oran Agra authored
    Remove read-only flag from non-keyspace cmds, different approach for EXEC to propagate MULTI (#8216)
    
    In the distant history there was only the read flag for commands, and whatever
    command that didn't have the read flag was a write one.
    Then we added the write flag, but some portions of the code still used !read
    Also some commands that don't work on the keyspace at all, still have the read
    flag.
    
    Changes in this commit:
    1. remove the read-only flag from TIME, ECHO, ROLE and LASTSAVE
    
    2. EXEC command used to decides if it should propagate a MULTI by looking at
       the command flags (!read & !admin).
       When i was about to change it to look at the write flag instead, i realized
       that this would cause it not to propagate a MULTI for PUBLISH, EVAL, and
       SCRIPT, all 3 are not marked as either a read command or a write one (as
       they should), but all 3 are calling forceCommandPropagation.
    
       So instead of introducing a new flag to denote a command that "writes" but
       not into the keyspace, and still needs propagation, i decided to rely on
       the forceCommandPropagation, and just fix the code to propagate MULTI when
       needed rather than depending on the command flags at all.
    
       The implication of my change then is that now it won't decide to propagate
       MULTI when it sees one of these: SELECT, PING, INFO, COMMAND, TIME and
       other commands which are neither read nor write.
    
    3. Changing getNodeByQuery and clusterRedirectBlockedClientIfNeeded in
       cluster.c to look at !write rather than read flag.
       This should have no implications, since these code paths are only reachable
       for commands which access keys, and these are always marked as either read
       or write.
    
    This commit improve MULTI propagation tests, for modules and a bunch of
    other special cases, all of which used to pass already before that commit.
    the only one that test change that uncovered a change of behavior is the
    one that DELs a non-existing key, it used to propagate an empty
    multi-exec block, and no longer does.
    411c18bb
multi.c 14.6 KB