Made brpoplpush and spop with count atomic when replicating#6152
Made brpoplpush and spop with count atomic when replicating#6152madolson wants to merge 1 commit intoredis:unstablefrom
Conversation
|
@soloestoy This might be relevant to you guys. |
|
Hi @madolson it's a good catch and your fix makes sense, and I do really meet the problem that replication breaks atomicity of command in our product environment. But we didn't fix it just in |
|
PR is good, but waiting @soloestoy new inputs. Putting in Redis 6 milestone. |
|
Sorry for the late reply, here is my way, I use the But I'm not sure if it has conflict with module, if you think this way is OK I can open a PR to push the whole codes. diff --git a/src/server.c b/src/server.c
index 4de5fb7..26cc316 100644
--- a/src/server.c
+++ b/src/server.c
@@ -2878,6 +2878,18 @@ void call(client *c, int flags) {
redisOp *rop;
if (flags & CMD_CALL_PROPAGATE) {
+ int multi_emitted = 0;
+ /* Wrap the commands in server.also_propagate array.
+ * Notice that: if we are already in MULIT context, it's unnecessary
+ * to wrap it again, in case the nested MULIT/EXEC.
+ * BTW, if the rewritten is only one command, it's unnecessary to wrap
+ * it again, because the single command still keep its atomicity. */
+ if (server.also_propagate.numops > 1 && !(c->flags & CLIENT_MULTI)) {
+ execCommandPropagateMulti(c);
+ multi_emitted = 1;
+ }
+
for (j = 0; j < server.also_propagate.numops; j++) {
rop = &server.also_propagate.ops[j];
int target = rop->target;
@@ -2887,6 +2899,11 @@ void call(client *c, int flags) {
if (target)
propagate(c, rop->cmd,rop->dbid,rop->argv,rop->argc,target);
}
+
+ if (multi_emitted) {
+ execCommandPropagateExec(c);
+ }
}
redisOpArrayFree(&server.also_propagate);
} |
|
I think the proposal that soloestoy suggested is better, we thought it would originally be a little intrusive to submit a PR for so we opted for the more tactical fix. the RPOPLPUSH also needs to be modified in order to support that, since it doesn't use also_propagate. |
|
Thanks, merged @soloestoy PR, closing this. |
3 participants
Two locations in Redis where commands that are replicated as multiple commands are not done so atomically. This can lead to a state on a replica, or AOF in some cases, that was never seen on the master if the output is not fully flushed.
The SPOP was converted to a transactions of SREM commands incase you have a large set and are dumping a lot of items because you might overwhelm the querybuf if you send it as a single SREM command. This could be optimized more.
For the blocking brpoplpush, I don't entirely know why it's not propagated as just an rpoplpush, so I just stuck with the original implementation, but that may be something that is a simpler fix.