<feed xmlns='http://www.w3.org/2005/Atom'>
<title>delta/redis.git/src/server.h, 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>AOF check utility: ability to check files with RDB preamble.</title>
<updated>2017-07-10T11:38:23+00:00</updated>
<author>
<name>antirez</name>
<email>antirez@gmail.com</email>
</author>
<published>2017-07-10T11:38:23+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/redis.git/commit/?id=fc7ecd8d359160c5b74aa86c09eefdf4e9e2d653'/>
<id>fc7ecd8d359160c5b74aa86c09eefdf4e9e2d653</id>
<content type='text'>
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
</pre>
</div>
</content>
</entry>
<entry>
<title>Modules: DEBUG DIGEST interface.</title>
<updated>2017-07-06T09:04:46+00:00</updated>
<author>
<name>antirez</name>
<email>antirez@gmail.com</email>
</author>
<published>2017-07-06T08:29:19+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/redis.git/commit/?id=51ffd062d37b370884e21cc70b20264ff4060dc8'/>
<id>51ffd062d37b370884e21cc70b20264ff4060dc8</id>
<content type='text'>
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
</pre>
</div>
</content>
</entry>
<entry>
<title>Added GEORADIUS(BYMEMBER)_RO variants for read-only operations.</title>
<updated>2017-06-30T08:03:37+00:00</updated>
<author>
<name>antirez</name>
<email>antirez@gmail.com</email>
</author>
<published>2017-06-30T08:03:37+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/redis.git/commit/?id=f8547e53f0d80b4cd2a84c6d39d1e08a470f0a2c'/>
<id>f8547e53f0d80b4cd2a84c6d39d1e08a470f0a2c</id>
<content type='text'>
Issue #4084 shows how for a design error, GEORADIUS is a write command
because of the STORE option. Because of this it does not work
on readonly slaves, gets redirected to masters in Redis Cluster even
when the connection is in READONLY mode and so forth.

To break backward compatibility at this stage, with Redis 4.0 to be in
advanced RC state, is problematic for the user base. The API can be
fixed into the unstable branch soon if we'll decide to do so in order to
be more consistent, and reease Redis 5.0 with this incompatibility in
the future. This is still unclear.

However, the ability to scale GEO queries in slaves easily is too
important so this commit adds two read-only variants to the GEORADIUS
and GEORADIUSBYMEMBER command: GEORADIUS_RO and GEORADIUSBYMEMBER_RO.
The commands are exactly as the original commands, but they do not
accept the STORE and STOREDIST options.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
Issue #4084 shows how for a design error, GEORADIUS is a write command
because of the STORE option. Because of this it does not work
on readonly slaves, gets redirected to masters in Redis Cluster even
when the connection is in READONLY mode and so forth.

To break backward compatibility at this stage, with Redis 4.0 to be in
advanced RC state, is problematic for the user base. The API can be
fixed into the unstable branch soon if we'll decide to do so in order to
be more consistent, and reease Redis 5.0 with this incompatibility in
the future. This is still unclear.

However, the ability to scale GEO queries in slaves easily is too
important so this commit adds two read-only variants to the GEORADIUS
and GEORADIUSBYMEMBER command: GEORADIUS_RO and GEORADIUSBYMEMBER_RO.
The commands are exactly as the original commands, but they do not
accept the STORE and STOREDIST options.
</pre>
</div>
</content>
</entry>
<entry>
<title>RDB modules values serialization format version 2.</title>
<updated>2017-06-27T11:19:16+00:00</updated>
<author>
<name>antirez</name>
<email>antirez@gmail.com</email>
</author>
<published>2017-06-27T11:09:33+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/redis.git/commit/?id=365dd037dcc00249c7631caac82c49a9c0c8c0f6'/>
<id>365dd037dcc00249c7631caac82c49a9c0c8c0f6</id>
<content type='text'>
The original RDB serialization format was not parsable without the
module loaded, becuase the structure was managed only by the module
itself. Moreover RDB is a streaming protocol in the sense that it is
both produce di an append-only fashion, and is also sometimes directly
sent to the socket (in the case of diskless replication).

The fact that modules values cannot be parsed without the relevant
module loaded is a problem in many ways: RDB checking tools must have
loaded modules even for doing things not involving the value at all,
like splitting an RDB into N RDBs by key or alike, or just checking the
RDB for sanity.

In theory module values could be just a blob of data with a prefixed
length in order for us to be able to skip it. However prefixing the values
with a length would mean one of the following:

1. To be able to write some data at a previous offset. This breaks
stremaing.
2. To bufferize values before outputting them. This breaks performances.
3. To have some chunked RDB output format. This breaks simplicity.

Moreover, the above solution, still makes module values a totally opaque
matter, with the fowllowing problems:

1. The RDB check tool can just skip the value without being able to at
least check the general structure. For datasets composed mostly of
modules values this means to just check the outer level of the RDB not
actually doing any checko on most of the data itself.
2. It is not possible to do any recovering or processing of data for which a
module no longer exists in the future, or is unknown.

So this commit implements a different solution. The modules RDB
serialization API is composed if well defined calls to store integers,
floats, doubles or strings. After this commit, the parts generated by
the module API have a one-byte prefix for each of the above emitted
parts, and there is a final EOF byte as well. So even if we don't know
exactly how to interpret a module value, we can always parse it at an
high level, check the overall structure, understand the types used to
store the information, and easily skip the whole value.

The change is backward compatible: older RDB files can be still loaded
since the new encoding has a new RDB type: MODULE_2 (of value 7).
The commit also implements the ability to check RDB files for sanity
taking advantage of the new feature.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
The original RDB serialization format was not parsable without the
module loaded, becuase the structure was managed only by the module
itself. Moreover RDB is a streaming protocol in the sense that it is
both produce di an append-only fashion, and is also sometimes directly
sent to the socket (in the case of diskless replication).

The fact that modules values cannot be parsed without the relevant
module loaded is a problem in many ways: RDB checking tools must have
loaded modules even for doing things not involving the value at all,
like splitting an RDB into N RDBs by key or alike, or just checking the
RDB for sanity.

In theory module values could be just a blob of data with a prefixed
length in order for us to be able to skip it. However prefixing the values
with a length would mean one of the following:

1. To be able to write some data at a previous offset. This breaks
stremaing.
2. To bufferize values before outputting them. This breaks performances.
3. To have some chunked RDB output format. This breaks simplicity.

Moreover, the above solution, still makes module values a totally opaque
matter, with the fowllowing problems:

1. The RDB check tool can just skip the value without being able to at
least check the general structure. For datasets composed mostly of
modules values this means to just check the outer level of the RDB not
actually doing any checko on most of the data itself.
2. It is not possible to do any recovering or processing of data for which a
module no longer exists in the future, or is unknown.

So this commit implements a different solution. The modules RDB
serialization API is composed if well defined calls to store integers,
floats, doubles or strings. After this commit, the parts generated by
the module API have a one-byte prefix for each of the above emitted
parts, and there is a final EOF byte as well. So even if we don't know
exactly how to interpret a module value, we can always parse it at an
high level, check the overall structure, understand the types used to
store the information, and easily skip the whole value.

The change is backward compatible: older RDB files can be still loaded
since the new encoding has a new RDB type: MODULE_2 (of value 7).
The commit also implements the ability to check RDB files for sanity
taking advantage of the new feature.
</pre>
</div>
</content>
</entry>
<entry>
<title>Fix set with ex/px option when propagated to aof</title>
<updated>2017-06-16T09:51:38+00:00</updated>
<author>
<name>xuzhou</name>
<email>xuzhou1@jd.com</email>
</author>
<published>2017-06-16T09:51:38+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/redis.git/commit/?id=530fcf868786950fbd07f294dd73f9f9bd631cb6'/>
<id>530fcf868786950fbd07f294dd73f9f9bd631cb6</id>
<content type='text'>
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
</pre>
</div>
</content>
</entry>
<entry>
<title>Implement getKeys procedure for georadius and georadiusbymember</title>
<updated>2017-06-14T16:15:48+00:00</updated>
<author>
<name>Qu Chen</name>
<email>quchen@amazon.com</email>
</author>
<published>2017-04-07T22:31:11+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/redis.git/commit/?id=4740424049173cf139b79a79df97878ab5f76f95'/>
<id>4740424049173cf139b79a79df97878ab5f76f95</id>
<content type='text'>
commands.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
commands.
</pre>
</div>
</content>
</entry>
<entry>
<title>Modules TSC: use atomic var for server.unixtime.</title>
<updated>2017-05-10T08:04:16+00:00</updated>
<author>
<name>antirez</name>
<email>antirez@gmail.com</email>
</author>
<published>2017-05-10T08:01:06+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/redis.git/commit/?id=1f598fc2bb6975417751486405303d98246bb7bc'/>
<id>1f598fc2bb6975417751486405303d98246bb7bc</id>
<content type='text'>
This avoids Helgrind complaining, but we are actually not using
atomicGet() to get the unixtime value for now: too many places where it
is used and given tha time_t is word-sized it should be safe in all the
archs we support as it is.

On the other hand, Helgrind, when Redis is compiled with "make helgrind"
in order to force the __sync macros, will detect the write in
updateCachedTime() as a read (because atomic functions are used) and
will not complain about races.

This commit also includes minor refactoring of mutex initializations and
a "helgrind" target in the Makefile.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
This avoids Helgrind complaining, but we are actually not using
atomicGet() to get the unixtime value for now: too many places where it
is used and given tha time_t is word-sized it should be safe in all the
archs we support as it is.

On the other hand, Helgrind, when Redis is compiled with "make helgrind"
in order to force the __sync macros, will detect the write in
updateCachedTime() as a read (because atomic functions are used) and
will not complain about races.

This commit also includes minor refactoring of mutex initializations and
a "helgrind" target in the Makefile.
</pre>
</div>
</content>
</entry>
<entry>
<title>Modules TSC: Add mutex for server.lruclock.</title>
<updated>2017-05-09T14:32:49+00:00</updated>
<author>
<name>antirez</name>
<email>antirez@gmail.com</email>
</author>
<published>2017-05-09T14:32:49+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/redis.git/commit/?id=9390c384b88de6b2363c3f33ba42bd25c1c3346d'/>
<id>9390c384b88de6b2363c3f33ba42bd25c1c3346d</id>
<content type='text'>
Only useful for when no atomic builtins are available.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
Only useful for when no atomic builtins are available.
</pre>
</div>
</content>
</entry>
<entry>
<title>Modules TSC: Improve inter-thread synchronization.</title>
<updated>2017-05-09T09:57:09+00:00</updated>
<author>
<name>antirez</name>
<email>antirez@gmail.com</email>
</author>
<published>2017-05-09T09:57:09+00:00</published>
<link rel='alternate' type='text/html' href='http://git.baserock.org/cgit/delta/redis.git/commit/?id=ece658713b659513e2c43a9498da286cafec17dd'/>
<id>ece658713b659513e2c43a9498da286cafec17dd</id>
<content type='text'>
More work to do with server.unixtime and similar. Need to write Helgrind
suppression file in order to suppress the valse positives.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
More work to do with server.unixtime and similar. Need to write Helgrind
suppression file in order to suppress the valse positives.
</pre>
</div>
</content>
</entry>
</feed>
