Unverified Commit 7ac21307 authored by guybe7's avatar guybe7 Committed by GitHub
Browse files

Sort out mess around propagation and MULTI/EXEC (#9890)

The mess:
Some parts use alsoPropagate for late propagation, others using an immediate one (propagate()),
causing edge cases, ugly/hacky code, and the tendency for bugs

The basic idea is that all commands are propagated via alsoPropagate (i.e. added to a list) and the
top-most call() is responsible for going over that list and actually propagating them (and wrapping
them in MULTI/EXEC if there's more than one command). This is done in the new function,
propagatePendingCommands.

Callers to propagatePendingCommands:
1. top-most call() (we want all nested call()s to add to the also_propagate array and just the top-most
   one to propagate them) - via `afterCommand`
2. handleClientsBlockedOnKeys: it is out of call() context and it may propagate stuff - via `afterCommand`. 
3. handleClientsBlockedOnKeys edge case: if the looked-up key is already expired, we will propagate the
   expire but will not unblock any client so `afterCommand` isn't called. in that case, we have to propagate
   the deletion explicitly.
4. cron stuff: active-expire and eviction may also propagate stuff
5. modules: the module API allows to propagate stuff from just about anywhere (timers, keyspace notifications,
   threads). I could have tried to catch all the out-of-call-context places but it seemed easier to handle it in one
   place: when we free the context. in the spirit of what was done in call(), only the top-most freeing of a module
   context may cause propagation.
6. modules: when using a thread-safe ctx it's not clear when/if the ctx will be freed. we do know that the module
   must lock the GIL before calling RM_Replicate/RM_Call so we propagate the pending commands when
   releasing the GIL.

A "known limitation", which were actually a bug, was fixed because of this commit (see propagate.tcl):
   When using a mix of RM_Call with `!` and RM_Replicate, the command would propagate out-of-order:
   first all the commands from RM_Call, and then the ones from RM_Replicate

Another thing worth mentioning is that if, in the past, a client would issue a MULTI/EXEC with just one
write command the server would blindly propagate the MULTI/EXEC too, even though it's redundant.
not anymore.

This commit renames propagate() to propagateNow() in order to cause conflicts in pending PRs.
propagatePendingCommands is the only caller of propagateNow, which is now a static, internal helper function.

Optimizations:
1. alsoPropagate will not add stuff to also_propagate if there's no AOF and replicas
2. alsoPropagate reallocs also_propagagte exponentially, to save calls to memmove

Bugfixes:
1. CONFIG SET can create evictions, sending notifications which can cause to dirty++ with modules.
   we need to prevent it from propagating to AOF/replicas
2. We need to set current_client in RM_Call. buggy scenario:
   - CONFIG SET maxmemory, eviction notifications, module hook calls RM_Call
   - assertion in lookupKey crashes, because current_client has CONFIG SET, which isn't CMD_WRITE
3. minor: in eviction, call propagateDeletion after notification, like active-expire and all commands
   (we always send a notification before propagating the command)
parent b7567394
...@@ -306,20 +306,69 @@ start_server {tags {"multi"}} { ...@@ -306,20 +306,69 @@ start_server {tags {"multi"}} {
r exec r exec
} {11} } {11}
test {MULTI / EXEC is propagated correctly (single write command)} { test {MULTI / EXEC is not propagated (single write command)} {
set repl [attach_to_replication_stream] set repl [attach_to_replication_stream]
r multi r multi
r set foo bar r set foo bar
r exec r exec
r set foo2 bar
assert_replication_stream $repl { assert_replication_stream $repl {
{select *} {select *}
{multi}
{set foo bar} {set foo bar}
{set foo2 bar}
}
close_replication_stream $repl
} {} {needs:repl}
test {MULTI / EXEC is propagated correctly (multiple commands)} {
set repl [attach_to_replication_stream]
r multi
r set foo{t} bar
r get foo{t}
r set foo2{t} bar2
r get foo2{t}
r set foo3{t} bar3
r get foo3{t}
r exec
assert_replication_stream $repl {
{select *}
{multi}
{set foo{t} bar}
{set foo2{t} bar2}
{set foo3{t} bar3}
{exec} {exec}
} }
close_replication_stream $repl close_replication_stream $repl
} {} {needs:repl} } {} {needs:repl}
test {MULTI / EXEC is propagated correctly (multiple commands with SELECT)} {
set repl [attach_to_replication_stream]
r multi
r select 1
r set foo{t} bar
r get foo{t}
r select 2
r set foo2{t} bar2
r get foo2{t}
r select 3
r set foo3{t} bar3
r get foo3{t}
r exec
assert_replication_stream $repl {
{select *}
{multi}
{set foo{t} bar}
{select *}
{set foo2{t} bar2}
{select *}
{set foo3{t} bar3}
{exec}
}
close_replication_stream $repl
} {} {needs:repl singledb:skip}
test {MULTI / EXEC is propagated correctly (empty transaction)} { test {MULTI / EXEC is propagated correctly (empty transaction)} {
set repl [attach_to_replication_stream] set repl [attach_to_replication_stream]
r multi r multi
...@@ -365,6 +414,25 @@ start_server {tags {"multi"}} { ...@@ -365,6 +414,25 @@ start_server {tags {"multi"}} {
close_replication_stream $repl close_replication_stream $repl
} {} {needs:repl} } {} {needs:repl}
test {MULTI / EXEC with REPLICAOF} {
# This test verifies that if we demote a master to replica inside a transaction, the
# entire transaction is not propagated to the already-connected replica
set repl [attach_to_replication_stream]
r set foo bar
r multi
r set foo2 bar
r replicaof localhost 9999
r set foo3 bar
r exec
catch {r set foo4 bar} e
assert_match {READONLY*} $e
assert_replication_stream $repl {
{select *}
{set foo bar}
}
r replicaof no one
} {OK} {needs:repl cluster:skip}
test {DISCARD should not fail during OOM} { test {DISCARD should not fail during OOM} {
set rd [redis_deferring_client] set rd [redis_deferring_client]
$rd config set maxmemory 1 $rd config set maxmemory 1
...@@ -585,16 +653,13 @@ start_server {tags {"multi"}} { ...@@ -585,16 +653,13 @@ start_server {tags {"multi"}} {
test {MULTI propagation of PUBLISH} { test {MULTI propagation of PUBLISH} {
set repl [attach_to_replication_stream] set repl [attach_to_replication_stream]
# make sure that PUBLISH inside MULTI is propagated in a transaction
r multi r multi
r publish bla bla r publish bla bla
r exec r exec
assert_replication_stream $repl { assert_replication_stream $repl {
{select *} {select *}
{multi}
{publish bla bla} {publish bla bla}
{exec}
} }
close_replication_stream $repl close_replication_stream $repl
} {} {needs:repl cluster:skip} } {} {needs:repl cluster:skip}
...@@ -611,9 +676,7 @@ start_server {tags {"multi"}} { ...@@ -611,9 +676,7 @@ start_server {tags {"multi"}} {
assert_replication_stream $repl { assert_replication_stream $repl {
{select *} {select *}
{multi}
{set foo bar} {set foo bar}
{exec}
} }
close_replication_stream $repl close_replication_stream $repl
} {} {needs:repl} } {} {needs:repl}
...@@ -628,9 +691,7 @@ start_server {tags {"multi"}} { ...@@ -628,9 +691,7 @@ start_server {tags {"multi"}} {
assert_replication_stream $repl { assert_replication_stream $repl {
{select *} {select *}
{multi}
{set bar bar} {set bar bar}
{exec}
} }
close_replication_stream $repl close_replication_stream $repl
} {} {needs:repl} } {} {needs:repl}
...@@ -646,32 +707,36 @@ start_server {tags {"multi"}} { ...@@ -646,32 +707,36 @@ start_server {tags {"multi"}} {
assert_replication_stream $repl { assert_replication_stream $repl {
{select *} {select *}
{multi}
{set foo bar} {set foo bar}
{exec}
} }
close_replication_stream $repl close_replication_stream $repl
} {} {need:repl} } {} {need:repl}
tags {"stream"} { tags {"stream"} {
test {MULTI propagation of XREADGROUP} { test {MULTI propagation of XREADGROUP} {
# stream is a special case because it calls propagate() directly for XREADGROUP
set repl [attach_to_replication_stream] set repl [attach_to_replication_stream]
r XADD mystream * foo bar r XADD mystream * foo bar
r XADD mystream * foo2 bar2
r XADD mystream * foo3 bar3
r XGROUP CREATE mystream mygroup 0 r XGROUP CREATE mystream mygroup 0
# make sure the XCALIM (propagated by XREADGROUP) is indeed inside MULTI/EXEC # make sure the XCALIM (propagated by XREADGROUP) is indeed inside MULTI/EXEC
r multi r multi
r XREADGROUP GROUP mygroup consumer1 COUNT 2 STREAMS mystream ">"
r XREADGROUP GROUP mygroup consumer1 STREAMS mystream ">" r XREADGROUP GROUP mygroup consumer1 STREAMS mystream ">"
r exec r exec
assert_replication_stream $repl { assert_replication_stream $repl {
{select *} {select *}
{xadd *} {xadd *}
{xadd *}
{xadd *}
{xgroup CREATE *} {xgroup CREATE *}
{multi} {multi}
{xclaim *} {xclaim *}
{xclaim *}
{xclaim *}
{exec} {exec}
} }
close_replication_stream $repl close_replication_stream $repl
......
...@@ -508,7 +508,7 @@ start_server {tags {"scripting"}} { ...@@ -508,7 +508,7 @@ start_server {tags {"scripting"}} {
assert {[r eval {return redis.call('spop', 'myset')} 0] ne {}} assert {[r eval {return redis.call('spop', 'myset')} 0] ne {}}
assert {[r eval {return redis.call('spop', 'myset', 1)} 0] ne {}} assert {[r eval {return redis.call('spop', 'myset', 1)} 0] ne {}}
assert {[r eval {return redis.call('spop', KEYS[1])} 1 myset] ne {}} assert {[r eval {return redis.call('spop', KEYS[1])} 1 myset] ne {}}
#this one below should be replicated by an empty MULTI/EXEC # this one below should not be replicated
assert {[r eval {return redis.call('spop', KEYS[1])} 1 myset] eq {}} assert {[r eval {return redis.call('spop', KEYS[1])} 1 myset] eq {}}
r set trailingkey 1 r set trailingkey 1
assert_replication_stream $repl { assert_replication_stream $repl {
...@@ -516,17 +516,9 @@ start_server {tags {"scripting"}} { ...@@ -516,17 +516,9 @@ start_server {tags {"scripting"}} {
{sadd *} {sadd *}
{del *} {del *}
{sadd *} {sadd *}
{multi}
{srem myset *} {srem myset *}
{exec}
{multi}
{srem myset *} {srem myset *}
{exec}
{multi}
{srem myset *} {srem myset *}
{exec}
{multi}
{exec}
{set *} {set *}
} }
close_replication_stream $repl close_replication_stream $repl
...@@ -555,9 +547,7 @@ start_server {tags {"scripting"}} { ...@@ -555,9 +547,7 @@ start_server {tags {"scripting"}} {
assert_replication_stream $repl { assert_replication_stream $repl {
{select *} {select *}
{set *} {set *}
{multi}
{pexpireat expirekey *} {pexpireat expirekey *}
{exec}
} }
close_replication_stream $repl close_replication_stream $repl
} {} {need:repl} } {} {need:repl}
......
...@@ -1015,7 +1015,8 @@ foreach {pop} {BLPOP BLMPOP_LEFT} { ...@@ -1015,7 +1015,8 @@ foreach {pop} {BLPOP BLMPOP_LEFT} {
$rd read $rd read
} {k hello} {singledb:skip} } {k hello} {singledb:skip}
test {SWAPDB awakes blocked client, but the key already expired} { test {SWAPDB wants to wake blocked client, but the key already expired} {
set repl [attach_to_replication_stream]
r flushall r flushall
r debug set-active-expire 0 r debug set-active-expire 0
r select 1 r select 1
...@@ -1036,6 +1037,55 @@ foreach {pop} {BLPOP BLMPOP_LEFT} { ...@@ -1036,6 +1037,55 @@ foreach {pop} {BLPOP BLMPOP_LEFT} {
assert_match "*flags=b*" [r client list id $id] assert_match "*flags=b*" [r client list id $id]
r client unblock $id r client unblock $id
assert_equal {} [$rd read] assert_equal {} [$rd read]
assert_replication_stream $repl {
{select *}
{flushall}
{select 1}
{rpush k hello}
{pexpireat k *}
{swapdb 1 9}
{select 9}
{del k}
}
close_replication_stream $repl
# Restore server and client state
r debug set-active-expire 1
r select 9
} {OK} {singledb:skip needs:debug}
test {MULTI + LPUSH + EXPIRE + DEBUG SLEEP on blocked client, key already expired} {
set repl [attach_to_replication_stream]
r flushall
r debug set-active-expire 0
set rd [redis_deferring_client]
$rd client id
set id [$rd read]
$rd brpop k 0
wait_for_blocked_clients_count 1
r multi
r rpush k hello
r pexpire k 100
r debug sleep 0.2
r exec
# The EXEC command tries to awake the blocked client, but it remains
# blocked because the key is expired. Check that the deferred client is
# still blocked. Then unblock it.
assert_match "*flags=b*" [r client list id $id]
r client unblock $id
assert_equal {} [$rd read]
assert_replication_stream $repl {
{select *}
{flushall}
{multi}
{rpush k hello}
{pexpireat k *}
{exec}
{del k}
}
close_replication_stream $repl
# Restore server and client state # Restore server and client state
r debug set-active-expire 1 r debug set-active-expire 1
r select 9 r select 9
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment