repl_scriptcache_dict 字典记录了自己已经将哪些脚本传播给了所有服务器
这个理解对吗?

repl_scriptcache_dict 字典记录了自己已经将哪些脚本传播给了所有服务器
这个理解对吗?

@cqyisbug repl_scriptcache_dict records scripts those converted from EVALSHA to EVAL. It means that scripts never touched by EVALSHA are not recorded.

    if (evalsha && !server.lua_replicate_commands) {
        if (!replicationScriptCacheExists(c->argv[1]->ptr)) {
            /* This script is not in our script cache, replicate it as
             * EVAL, then add it into the script cache, as from now on
             * slaves and AOF know about it. */
            robj *script = dictFetchValue(server.lua_scripts,c->argv[1]->ptr);

            replicationScriptCacheAdd(c->argv[1]->ptr);
            serverAssertWithInfo(c,NULL,script != NULL);
            rewriteClientCommandArgument(c,0,
                resetRefCount(createStringObject("EVAL",4)));
            rewriteClientCommandArgument(c,1,script);
            forceCommandPropagation(c,PROPAGATE_REPL|PROPAGATE_AOF);
        }
    }

@soloestoy thanks for your advice. ^_^

Moreover , if we apply SCRIPT LOAD "redis.call('keys','*')" and then apply EVALSHA on master, master will propagate EVAL "redis.call('keys','*')" 0 to slave, because of the code above.

But it's a waste of time that a slave execute EVAL "redis.call('keys','*')" 0 from master, @antirez

Ping @antirez

Hello @soloestoy, I've to study the PR, just had time to scan it when you posted it initially: my impression is that this PR attempts to prevent users from doing Very Silly Things (TM) :-) Like going and poke with the scripting cache of a slave, by doing SCRIPT FLUSH/LOAD in a slave. In general as you can see in all the Redis design, we don't try to avoid that users can shoot themselves in the feet... So that would not be a good fit. However I've to read the code and evaluate the PR with more care, this is just to give a general feedback about this kind of changes. Thanks.

Hi @antirez , thanks for your reply.

In general as you can see in all the Redis design, we don't try to avoid that users can shoot themselves in the feet...

I totally agree with you, I don't care users' wrong and silly behavior neither.

My main idea is to implement a new propagation for Lua scripts, the current Lua scripts propagation is a bit complicated and there is a waste of memory and cpu time.

Firstly, let's see the current Lua scripts propagation mechanism(here we ignore redis. replicate_commands for the time being):

1. When does redis propagate Lua scripts explicitly?

   SCRIPT LOAD command propagates Lua script to slaves and aof, and adds the script to server.lua_scripts.

   At the same time it means that we can apply EVALSHA on redis even after rebooting or failover.

2. When does redis propagate Lua scripts implicitly?

   i. EVAL command with WRITE Lua script.

   • Then redis propagates the Lua script to slaves and aof, and adds the script to server.lua_scripts as what SCRIPT LOAD does.

   • Problem 1: we cannot propagate READ Lua script by EVAL, and then we cannot apply EVALSHA after rebooting and failover.

   ii. EVAHSHA command with new sha.

   • As we know the new sha is not in server.repl_scriptcache_dict(but it is already in server.lua_scipts), so we have to rewrite EVALSHA as EVAL with the original script, and propagate it to slaves and aof.

   • Problem 2: we have to propagate the original script even it is already propagated by SCRIPT LOAD or EVAL, it's unnecessary.

   • Problem 3: we have to propagate the original script even it's a READ script like redis.call('keys','*'), it's a waste of time that a slave execute EVAL "redis.call('keys','*')" 0 or master load from aof.

   • Problem 4: replicationScriptCacheFlush will be called when aof rewrite and full resync, it means that all shas called by EVALSHA will be flushed then all EVALSHA have to propagate again.

   • Problem 5: old sha will be evicted when we reach max number of server.repl_scriptcache_fifo entries.

But remember that we already persist all Lua scripts into RDB file after Redis 4.0, so I'm thinking can we implement a new propagation for Lua scripts to make it simple and effective?

I think maybe Lua scripts should be regarded as a part of state of the instance as well. We can take the Lua scripts as data that not belong to any DB but belong to the whole instance. And then what I did is that:

1. Propagate EVAL command as SCRIPT LOAD when it's the first time we meet this script.

   It means that even READ scripts can be propagated to slaves and aof by EVAL command, and slaves could be same with master state.

2. Disable SCRIPT FLUSH|LOAD and EVAL command on slave.

   Because these two commands may break Lua scripting state.

3. Persist all Lua scripts in AOF and RDB file.

   Then slaves could be same with master's state and AOF file contains all Lua scripts even after rewrite.

4. Remove server.repl_scriptcache_dict and server.repl_scriptcache_fifo.

   We can guarantee that even after FULL RESYNC or AOF REWRITE redis can hold all Lua scripts, so these two are not needed any more. Scripts will not be propagated duplicated and slave will not execute READ Lua script from master.

Hello @soloestoy, I examined your proposal. In general those are good ideas, however given that we now have a plan to avoid all this complexity anyway, and switch to a feature where the only propagation mechanism is effects replication of scripts, people that want to use scripts in slaves will have to follow the protocol as usually... that is: EVALSHA and if the server replies with a no script error, use EVAL instead. So all the instances, including slaves, are unrelated to all the other instances from the POV of the scripts they have in memory. This is in general a very good simplification.

However, there is one of the points in your proposal that IMHO is a serious bug, and we should address it ASAP, regardless of the fact we are going to remove all this code in the future (hopefully), and is that read-only scripts, or aborted scripts (scripts with writes that abort before the first write is ever executed, so they are effectively read-only), are propagated anyway.

I believe that we should instead translate such scripts as SCRIPT LOAD in the slave here:

    if (evalsha && !server.lua_replicate_commands) {
        if (!replicationScriptCacheExists(c->argv[1]->ptr)) {
            /* This script is not in our script cache, replicate it as
             * EVAL, then add it into the script cache, as from now on
             * slaves and AOF know about it. */
            robj *script = dictFetchValue(server.lua_scripts,c->argv[1]->ptr);

            replicationScriptCacheAdd(c->argv[1]->ptr);
            serverAssertWithInfo(c,NULL,script != NULL);
            rewriteClientCommandArgument(c,0,
                resetRefCount(createStringObject("EVAL",4)));
            rewriteClientCommandArgument(c,1,script);
            forceCommandPropagation(c,PROPAGATE_REPL|PROPAGATE_AOF);
        }
    }

The above code, does not attempt to do any differentiation between scripts that produced changes, and scripts that instead were completely read-only. So my suggestion is that we do such change for now ignoring all the rest, and go forward with our plan to enable effects replication by default (with some hidden config to revert it if really needed). If such plan fails, and we see in the practice that users really need verbatim scripts replication, then we can re-examine the issue and understand what's the best approach to propagate the scripts, starting from your proposal. Does this make sense to you? Thanks for all your great work in this issue (and otherwise!).

Thanks your reply @antirez , I will analyze the bug you pointed carefully.

I agree with that we can go forward to enable effects replication by default, at the same time I'm trying to find a good way to propagate the verbatim scripts, it's not finished yet, maybe can finish these days.

no longer relevant, script propagation was deprecated in #9812