Multi-key commands about lookupKeyWrite and lookupKeyRead

[yangbodong22011]

Hi,
In Redis, when we operate the key, we need lookupKeyWrite or lookupKeyRead, but under the multi-key command, this flag is not strict.

E.g:

void sunionDiffGenericCommand(client *c, robj **setkeys, int setnum,
                              robj *dstkey, int op) {
    ...

    for (j = 0; j < setnum; j++) {
        robj *setobj = dstkey ?
            lookupKeyWrite(c->db,setkeys[j]) :
            lookupKeyRead(c->db,setkeys[j]);
        ...
    }
}

According to whether dstkey exists, use lookupKeyWrite or lookupKeyRead respectively, but should only use lookupKeyRead?

void zunionInterGenericCommand(client *c, robj *dstkey, int op) {
    ...
    /* read keys to be used for input */
    src = zcalloc(sizeof(zsetopsrc) * setnum);
    for (i = 0, j = 3; i < setnum; i++, j++) {
        robj *obj = lookupKeyWrite(c->db,c->argv[j]);
        
        ...
    }
}

Similarly, should lookupKeyRead also be used here?

If the judgment is based on the read and write attributes of the command itself, should georadius be modified to lookupKeyWrite?

{"georadius",georadiusCommand,-6,
     "write @geo",
     0,georadiusGetKeys,1,1,1,0,0,0},

void georadiusGeneric(client *c, int flags) {
    /* Look up the requested zset */
    robj *zobj = NULL;
    if ((zobj = lookupKeyReadOrReply(c, key, shared.emptyarray)) == NULL ||  // command is "write @geo"
        checkType(c, zobj, OBJ_ZSET)) {
        return;
    }
}

I think it should be decided according to the specific operation of this key, so the above should be changed to lookupKeyRead.

[soloestoy]

Makes sense, @oranagra what do you think?

[oranagra]

To be honest i never really understood that part and the reasoning behind it very well.

But the logic in sunionDiffGenericCommand makes it very clear what was the intention.
i.e. open the source keys with either read or write mode depending on if the command is a write or read one.

96ffb2fe97 (Pieter Noordhuis 2010-07-02 19:57:12 +0200  941)         robj *setobj = dstkey ?
96ffb2fe97 (Pieter Noordhuis 2010-07-02 19:57:12 +0200  942)             lookupKeyWrite(c->db,setkeys[j]) :
96ffb2fe97 (Pieter Noordhuis 2010-07-02 19:57:12 +0200  943)             lookupKeyRead(c->db,setkeys[j]);

In SUNION vs SUNIONSTORE is may be clear from the command flags, but in commands such as SORT and GEORADIUS we need to look for the STORE argument, which is what i did for SORT in this recent PR: #5390

I personally think we should continue in that path, and make the code consistent with itself, eventually we should be able to tell (and add statistics) of both considerations:

1. keys opened as part of read command vs write command.
2. keys opened for the purpose of reading, modifying, or overwriting them.

but most important is obviously not to have bugs 8-), so let's look at the differences between lookupKeyWrite and lookupKeyRead to see if something is missing.

putting aside key miss keyspace notification (which is a recent addition and may be just missing from lookupKeyWrite by mistake), and putting aside statistics.

It's a bit hard to read, but as far as i can tell, the only actual diff in behavior is that lookupKeyRead can return NULL on a replica if the current client is not the master, when the key already expired but not yet deleted by the master (so expireIfNeeded doesn't delete it). and even that only happens on pure READONLY commands.

So the only bugs that can happen by using the wrong lookup function are when there are reads executed directly to the replica, but for these (READONLY commands), we never even consider calling lookupKeyWrite.
And for non-READONLY commands, the only difference between calling lookupKeyRead and lookupKeyWrite would be statistics (and keymiss event). right?

@soloestoy @yossigo @guybe7 (@antirez) please validate my understanding / reasoning.
if we agree on that, i think there's a need for a small refactoring / renaming so that this is clearer by reading the code.

[guybe7]

@oranagra TBH i don't really see the point of knowing which "keys opened as part of read command vs write command"...
IMHO what we need to keep track of is only "keys opened for the purpose of reading, modifying, or overwriting them".
i'm not a Redis user so maybe i'm missing something...
i opened a related issue a while back: #6842 - it refers specifically to the issue with expired keys on a replica

[oranagra]

@guybe7 i also don't see why we wanna know if the open was part of a read or write, but clearly it mattered to Pieter, and maybe to Salvatore (merged my #5390), so for now i prefer keep that indication and not use it, rather than refactor it out of the code base.

i forgot about #6842, but i now realize that the problem there is not actually the use of lookupKeyRead vs lookupKeyWrite, but rather the different behavior for non-READONLY commands (which looks like a bug), more details there..

[soloestoy]

I agree with @guybe7 , the judgement of using lookupKeyRead or lookupKeyWrite should depend on the purpose instead of the command, cause write command may read some keys and modify the others.

Also, we need remove the check of CMD_READONLY in lookupKeyReadWithFlags I think.

[yossigo]

Trying to sum it up:

1. lookupKeyRead() and lookupKeyWrite() indicate what we intend to do with the key and should not be affected by the command itself.
2. Expired keys should never be hidden from commands originating from a master or AOF. Typically these would always be write commands (especially now that Lua scripts don't replicate) but the criteria is their origin and not whether or not they're writing.
3. We may want to preserve the RO/RW context of the command as an intermediate step just in case we missed something, but we can do it explicitly rather than rely on the command flags which is not always accurate.
   Does that make sense?

[yangbodong22011]

For the code modification, I want to confirm:

1. lookupKeyRead or lookupKeyWrite depend on the purpose instead of the command.
2. remove the check of CMD_READONLY in lookupKeyReadWithFlags.

Please tell me any other suggestions.