<feed xmlns='http://www.w3.org/2005/Atom'>
<title>delta/redis.git/src/multi.c, branch modules-replication</title>
<subtitle>github.com: antirez/redis.git
</subtitle>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/redis.git/'/>
<entry>
<title>Fix replication of SLAVEOF inside transaction.</title>
<updated>2017-07-12T09:07:28+00:00</updated>
<author>
<name>antirez</name>
<email>antirez@gmail.com</email>
</author>
<published>2017-07-12T09:07:28+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/redis.git/commit/?id=e74f0aa6d1ca4a7d0626125a7d9b5eb9cb713947'/>
<id>e74f0aa6d1ca4a7d0626125a7d9b5eb9cb713947</id>
<content type='text'>
In Redis 4.0 replication, with the introduction of PSYNC2, masters and
slaves replicate commands to cascading slaves and to the replication
backlog itself in a different way compared to the past.

Masters actually replicate the effects of client commands.
Slaves just propagate what they receive from masters.

This mechanism can cause problems when the configuration of an instance
is changed from master to slave inside a transaction. For instance
we could send to a master instance the following sequence:

    MULTI
    SLAVEOF 127.0.0.1 0
    EXEC
    SLAVEOF NO ONE

Before the fixes in this commit, the MULTI command used to be propagated
into the replication backlog, however after the SLAVEOF command the
instance is a slave, so the EXEC implementation failed to also propagate
the EXEC command. When the slaves of the above instance reconnected,
they were incrementally synchronized just sending a "MULTI". This put
the master client (in the slaves) into MULTI state, breaking the
replication.

Notably even Redis Sentinel uses the above approach in order to guarantee
that configuration changes are always performed together with rewrites
of the configuration and with clients disconnection. Sentiel does:

    MULTI
    SLAVEOF ...
    CONFIG REWRITE
    CLIENT KILL TYPE normal
    EXEC

So this was a really problematic issue. However even with the fix in
this commit, that will add the final EXEC to the replication stream in
case the instance was switched from master to slave during the
transaction, the result would be to increment the slave replication
offset, so a successive reconnection with the new master, will not
permit a successful partial resynchronization: no way the new master can
provide us with the backlog needed, we incremented our offset to a value
that the new master cannot have.

However the EXEC implementation waits to emit the MULTI, so that if the
commands inside the transaction actually do not need to be replicated,
no commands propagation happens at all. From multi.c:

    if (!must_propagate &amp;&amp; !(c-&gt;cmd-&gt;flags &amp; (CMD_READONLY|CMD_ADMIN))) {
	execCommandPropagateMulti(c);
	must_propagate = 1;
    }

The above code is already modified by this commit you are reading.
Now also ADMIN commands do not trigger the emission of MULTI. It is actually
not clear why we do not just check for CMD_WRITE... Probably I wrote it this
way in order to make the code more reliable: better to over-emit MULTI
than not emitting it in time.

So this commit should indeed fix issue #3836 (verified), however it looks
like some reconsideration of this code path is needed in the long term.

BONUS POINT: The reverse bug.

Even in a read only slave "B", in a replication setup like:

	A -&gt; B -&gt; C

There are commands without the READONLY nor the ADMIN flag, that are also
not flagged as WRITE commands. An example is just the PING command.

So if we send B the following sequence:

    MULTI
    PING
    SLAVEOF NO ONE
    EXEC

The result will be the reverse bug, where only EXEC is emitted, but not the
previous MULTI. However this apparently does not create problems in practice
but it is yet another acknowledge of the fact some work is needed here
in order to make this code path less surprising.

Note that there are many different approaches we could follow. For instance
MULTI/EXEC blocks containing administrative commands may be allowed ONLY
if all the commands are administrative ones, otherwise they could be
denined. When allowed, the commands could simply never be replicated at all.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
In Redis 4.0 replication, with the introduction of PSYNC2, masters and
slaves replicate commands to cascading slaves and to the replication
backlog itself in a different way compared to the past.

Masters actually replicate the effects of client commands.
Slaves just propagate what they receive from masters.

This mechanism can cause problems when the configuration of an instance
is changed from master to slave inside a transaction. For instance
we could send to a master instance the following sequence:

    MULTI
    SLAVEOF 127.0.0.1 0
    EXEC
    SLAVEOF NO ONE

Before the fixes in this commit, the MULTI command used to be propagated
into the replication backlog, however after the SLAVEOF command the
instance is a slave, so the EXEC implementation failed to also propagate
the EXEC command. When the slaves of the above instance reconnected,
they were incrementally synchronized just sending a "MULTI". This put
the master client (in the slaves) into MULTI state, breaking the
replication.

Notably even Redis Sentinel uses the above approach in order to guarantee
that configuration changes are always performed together with rewrites
of the configuration and with clients disconnection. Sentiel does:

    MULTI
    SLAVEOF ...
    CONFIG REWRITE
    CLIENT KILL TYPE normal
    EXEC

So this was a really problematic issue. However even with the fix in
this commit, that will add the final EXEC to the replication stream in
case the instance was switched from master to slave during the
transaction, the result would be to increment the slave replication
offset, so a successive reconnection with the new master, will not
permit a successful partial resynchronization: no way the new master can
provide us with the backlog needed, we incremented our offset to a value
that the new master cannot have.

However the EXEC implementation waits to emit the MULTI, so that if the
commands inside the transaction actually do not need to be replicated,
no commands propagation happens at all. From multi.c:

    if (!must_propagate &amp;&amp; !(c-&gt;cmd-&gt;flags &amp; (CMD_READONLY|CMD_ADMIN))) {
	execCommandPropagateMulti(c);
	must_propagate = 1;
    }

The above code is already modified by this commit you are reading.
Now also ADMIN commands do not trigger the emission of MULTI. It is actually
not clear why we do not just check for CMD_WRITE... Probably I wrote it this
way in order to make the code more reliable: better to over-emit MULTI
than not emitting it in time.

So this commit should indeed fix issue #3836 (verified), however it looks
like some reconsideration of this code path is needed in the long term.

BONUS POINT: The reverse bug.

Even in a read only slave "B", in a replication setup like:

	A -&gt; B -&gt; C

There are commands without the READONLY nor the ADMIN flag, that are also
not flagged as WRITE commands. An example is just the PING command.

So if we send B the following sequence:

    MULTI
    PING
    SLAVEOF NO ONE
    EXEC

The result will be the reverse bug, where only EXEC is emitted, but not the
previous MULTI. However this apparently does not create problems in practice
but it is yet another acknowledge of the fact some work is needed here
in order to make this code path less surprising.

Note that there are many different approaches we could follow. For instance
MULTI/EXEC blocks containing administrative commands may be allowed ONLY
if all the commands are administrative ones, otherwise they could be
denined. When allowed, the commands could simply never be replicated at all.
</pre>
</div>
</content>
</entry>
<entry>
<title>RDMF: More consistent define names.</title>
<updated>2015-07-27T12:37:58+00:00</updated>
<author>
<name>antirez</name>
<email>antirez@gmail.com</email>
</author>
<published>2015-07-27T07:41:48+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/redis.git/commit/?id=32f80e2f1bf42d0508f1114a9dddd91c4719eb8e'/>
<id>32f80e2f1bf42d0508f1114a9dddd91c4719eb8e</id>
<content type='text'>
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
</pre>
</div>
</content>
</entry>
<entry>
<title>RDMF: redisAssert -&gt; serverAssert.</title>
<updated>2015-07-26T13:29:53+00:00</updated>
<author>
<name>antirez</name>
<email>antirez@gmail.com</email>
</author>
<published>2015-07-26T13:29:53+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/redis.git/commit/?id=2d9e3eb107b6b6289d5e3a51a32b3a018c96103c'/>
<id>2d9e3eb107b6b6289d5e3a51a32b3a018c96103c</id>
<content type='text'>
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
</pre>
</div>
</content>
</entry>
<entry>
<title>RDMF: use client instead of redisClient, like Disque.</title>
<updated>2015-07-26T13:20:52+00:00</updated>
<author>
<name>antirez</name>
<email>antirez@gmail.com</email>
</author>
<published>2015-07-26T13:20:46+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/redis.git/commit/?id=554bd0e7bd81715e319cafda437ed2aebd44b6e9'/>
<id>554bd0e7bd81715e319cafda437ed2aebd44b6e9</id>
<content type='text'>
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
</pre>
</div>
</content>
</entry>
<entry>
<title>RDMF (Redis/Disque merge friendlyness) refactoring WIP 1.</title>
<updated>2015-07-26T13:17:18+00:00</updated>
<author>
<name>antirez</name>
<email>antirez@gmail.com</email>
</author>
<published>2015-07-26T13:14:57+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/redis.git/commit/?id=cef054e86856463d3e79d4a5048507377c85eab7'/>
<id>cef054e86856463d3e79d4a5048507377c85eab7</id>
<content type='text'>
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
</pre>
</div>
</content>
</entry>
<entry>
<title>Cleanup double semicolons</title>
<updated>2014-08-08T12:54:02+00:00</updated>
<author>
<name>Matt Stancliff</name>
<email>matt@genges.com</email>
</author>
<published>2014-08-01T19:42:50+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/redis.git/commit/?id=da0132638f15d81782c5543ed49a1d1f17594d9d'/>
<id>da0132638f15d81782c5543ed49a1d1f17594d9d</id>
<content type='text'>
Closes #1161
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
Closes #1161
</pre>
</div>
</content>
</entry>
<entry>
<title>No more trailing spaces in Redis source code.</title>
<updated>2014-06-26T16:48:40+00:00</updated>
<author>
<name>antirez</name>
<email>antirez@gmail.com</email>
</author>
<published>2014-06-26T16:48:40+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/redis.git/commit/?id=95b1979c321eb6353f75df892ab8be68cf8f9a77'/>
<id>95b1979c321eb6353f75df892ab8be68cf8f9a77</id>
<content type='text'>
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
</pre>
</div>
</content>
</entry>
<entry>
<title>Transactions: propagate MULTI/EXEC only when needed.</title>
<updated>2013-03-26T09:58:10+00:00</updated>
<author>
<name>antirez</name>
<email>antirez@gmail.com</email>
</author>
<published>2013-03-26T09:58:10+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/redis.git/commit/?id=71f3e7431700f55b88582301d5b36b52b37a0740'/>
<id>71f3e7431700f55b88582301d5b36b52b37a0740</id>
<content type='text'>
MULTI/EXEC is now propagated to the AOF / Slaves only once we encounter
the first command that is not a read-only one inside the transaction.

The old behavior was to always propagate an empty MULTI/EXEC block when
the transaction was composed just of read only commands, or even
completely empty. This created two problems:

1) It's a bandwidth waste in the replication link and a space waste
   inside the AOF file.

2) We used to always increment server.dirty to force the propagation of
   the EXEC command, resulting into triggering RDB saves more often
   than needed.

Note: even read-only commands may also trigger writes that will be
propagated, when we access a key that is found expired and Redis will
synthesize a DEL operation. However there is no need for this to stay
inside the transaction itself, but only to be ordered.

So for instance something like:

    MULTI
    GET foo
    SET key zap
    EXEC

May be propagated into:

    DEL foo
    MULTI
    SET key zap
    EXEC

While the DEL is outside the transaction, the commands are delivered in
the right order and it is not possible for other commands to be inserted
between DEL and MULTI.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
MULTI/EXEC is now propagated to the AOF / Slaves only once we encounter
the first command that is not a read-only one inside the transaction.

The old behavior was to always propagate an empty MULTI/EXEC block when
the transaction was composed just of read only commands, or even
completely empty. This created two problems:

1) It's a bandwidth waste in the replication link and a space waste
   inside the AOF file.

2) We used to always increment server.dirty to force the propagation of
   the EXEC command, resulting into triggering RDB saves more often
   than needed.

Note: even read-only commands may also trigger writes that will be
propagated, when we access a key that is found expired and Redis will
synthesize a DEL operation. However there is no need for this to stay
inside the transaction itself, but only to be ordered.

So for instance something like:

    MULTI
    GET foo
    SET key zap
    EXEC

May be propagated into:

    DEL foo
    MULTI
    SET key zap
    EXEC

While the DEL is outside the transaction, the commands are delivered in
the right order and it is not possible for other commands to be inserted
between DEL and MULTI.
</pre>
</div>
</content>
</entry>
<entry>
<title>Transactions: use discardTransaction() in EXEC implementation.</title>
<updated>2013-03-26T09:48:15+00:00</updated>
<author>
<name>antirez</name>
<email>antirez@gmail.com</email>
</author>
<published>2013-03-26T09:48:15+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/redis.git/commit/?id=02c269e27e846b904db80636d321686fa11695d4'/>
<id>02c269e27e846b904db80636d321686fa11695d4</id>
<content type='text'>
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
</pre>
</div>
</content>
</entry>
<entry>
<title>Transactions: use the propagate() API to propagate MULTI.</title>
<updated>2013-03-26T09:27:45+00:00</updated>
<author>
<name>antirez</name>
<email>antirez@gmail.com</email>
</author>
<published>2013-03-26T09:27:45+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/redis.git/commit/?id=2f49734029bb94c74466e13f517a480cdcf548df'/>
<id>2f49734029bb94c74466e13f517a480cdcf548df</id>
<content type='text'>
The behavior is the same, but the code is now cleaner and uses the
proper interface instead of dealing directly with AOF/replication
functions.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
The behavior is the same, but the code is now cleaner and uses the
proper interface instead of dealing directly with AOF/replication
functions.
</pre>
</div>
</content>
</entry>
</feed>
