1. 05 Oct, 2018 1 commit
  2. 11 Sep, 2018 2 commits
  3. 17 Jul, 2018 1 commit
    • Oran Agra's avatar
      fix rare replication stream corruption with disk-based replication · d5559898
      Oran Agra authored
      The slave sends \n keepalive messages to the master while parsing the rdb,
      and later sends REPLCONF ACK once a second. rarely, the master recives both
      a linefeed char and a REPLCONF in the same read, \n*3\r\n$8\r\nREPLCONF\r\n...
      and it tries to trim two chars (\r\n) from the query buffer,
      trimming the '*' from *3\r\n$8\r\nREPLCONF\r\n...
      
      then the master tries to process a command starting with '3' and replies to
      the slave a bunch of -ERR and one +OK.
      although the slave silently ignores these (prints a log message), this corrupts
      the replication offset at the slave since the slave increases the replication
      offset, and the master did not.
      
      other than the fix in processInlineBuffer, i did several other improvments
      while hunting this very rare bug.
      
      - when redis replies with "unknown command" it includes a portion of the
        arguments, not just the command name. so it would be easier to understand
        what was recived, in my case, on the slave side,  it was -ERR, but
        the "arguments" were the interesting part (containing info on the error).
      - about a year ago i added code in addReplyErrorLength to print the error to
        the log in case of a reply to master (since this string isn't actually
        trasmitted to the master), now changed that block to print a similar log
        message to indicate an error being sent from the master to the slave.
        note that the slave is marked as CLIENT_SLAVE only after PSYNC was received,
        so this will not cause any harm for REPLCONF, and will only indicate problems
        that are gonna corrupt the replication stream anyway.
      - two places were c->reply was emptied, and i wanted to reset sentlen
        this is a precaution (i did not actually see such a problem), since a
        non-zero sentlen will cause corruption to be transmitted on the socket.
      d5559898
  4. 16 Jul, 2018 1 commit
    • Oran Agra's avatar
      slave buffers were wasteful and incorrectly counted causing eviction · bf680b6f
      Oran Agra authored
      A) slave buffers didn't count internal fragmentation and sds unused space,
         this caused them to induce eviction although we didn't mean for it.
      
      B) slave buffers were consuming about twice the memory of what they actually needed.
      - this was mainly due to sdsMakeRoomFor growing to twice as much as needed each time
        but networking.c not storing more than 16k (partially fixed recently in 237a38737).
      - besides it wasn't able to store half of the new string into one buffer and the
        other half into the next (so the above mentioned fix helped mainly for small items).
      - lastly, the sds buffers had up to 30% internal fragmentation that was wasted,
        consumed but not used.
      
      C) inefficient performance due to starting from a small string and reallocing many times.
      
      what i changed:
      - creating dedicated buffers for reply list, counting their size with zmalloc_size
      - when creating a new reply node from, preallocate it to at least 16k.
      - when appending a new reply to the buffer, first fill all the unused space of the
        previous node before starting a new one.
      
      other changes:
      - expose mem_not_counted_for_evict info field for the benefit of the test suite
      - add a test to make sure slave buffers are counted correctly and that they don't cause eviction
      bf680b6f
  5. 03 Jul, 2018 2 commits
    • Jack Drogon's avatar
      Fix typo · 93238575
      Jack Drogon authored
      93238575
    • antirez's avatar
      Set repl_down_since to zero on state change. · 677d10b2
      antirez authored
      PR #5081 fixes an "interesting" bug about Redis Cluster failover but in
      general about the updating of repl_down_since, that is used in order to
      count the time a slave was left disconnected from its master.
      
      While the fix provided resolves the specific issue, in general the
      validity of repl_down_since is limited to states that are different
      than the state CONNECTED, and the disconnected time is set when the
      state is DISCONNECTED. However from CONNECTED to other states, the state
      machine must always go to DISCONNECTED first. So it makes sense to set
      the field to zero (since it is meaningless in that context) when the
      state is set to CONNECTED.
      677d10b2
  6. 30 Jun, 2018 1 commit
  7. 26 Jun, 2018 4 commits
  8. 06 Jun, 2018 1 commit
  9. 16 Jan, 2018 2 commits
  10. 05 Dec, 2017 1 commit
    • antirez's avatar
      add linkClient(): adds the client and caches the list node. · 62a4b817
      antirez authored
      We have this operation in two places: when caching the master and
      when linking a new client after the client creation. By having an API
      for this we avoid incurring in errors when modifying one of the two
      places forgetting the other. The function is also a good place where to
      document why we cache the linked list node.
      
      Related to #4497 and #4210.
      62a4b817
  11. 30 Nov, 2017 1 commit
  12. 24 Nov, 2017 1 commit
  13. 01 Nov, 2017 1 commit
    • zhaozhao.zz's avatar
      PSYNC2: safe free backlog when reach the time limit · 6ddf0ea2
      zhaozhao.zz authored
      When we free the backlog, we should use a new
      replication ID and clear the ID2. Since without
      backlog we can not increment master_repl_offset
      even do write commands, that may lead to inconsistency
      when we try to connect a "slave-before" master
      (if this master is our slave before, our replid
      equals the master's replid2). As the master have our
      history, so we can match the master's replid2 and
      second_replid_offset, that make partial sync work,
      but the data is inconsistent.
      6ddf0ea2
  14. 20 Sep, 2017 2 commits
    • antirez's avatar
      PSYNC2: More refinements related to #4316. · bb3b5ddd
      antirez authored
      bb3b5ddd
    • zhaozhao.zz's avatar
      PSYNC2: make persisiting replication info more solid · b541ccef
      zhaozhao.zz authored
      This commit is a reinforcement of commit c1c99e9f.
      
      1. Replication information can be stored when the RDB file is
      generated by a mater using server.slaveseldb when server.repl_backlog
      is not NULL, or set repl_stream_db be -1. That's safe, because
      NULL server.repl_backlog will trigger full synchronization,
      then master will send SELECT command to replicaiton stream.
      2. Only do rdbSave* when rsiptr is not NULL,
      if we do rdbSave* without rdbSaveInfo, slave will miss repl-stream-db.
      3. Save the replication informations also in the case of
      SAVE command, FLUSHALL command and DEBUG reload.
      b541ccef
  15. 19 Sep, 2017 2 commits
    • antirez's avatar
      PSYNC2: Fix the way replication info is saved/loaded from RDB. · c1c99e9f
      antirez authored
      This commit attempts to fix a number of bugs reported in #4316.
      They are related to the way replication info like replication ID,
      offsets, and currently selected DB in the master client, are stored
      and loaded by Redis. In order to avoid inconsistencies the changes in
      this commit try to enforce that:
      
      1. Replication information are only stored when the RDB file is
      generated by a slave that has a valid 'master' client, so that we can
      always extract the currently selected DB.
      2. When replication informations are persisted in the RDB file, all the
      info for a successful PSYNC or nothing is persisted.
      3. The RDB replication informations are only loaded if the instance is
      configured as a slave, otherwise a master can start with IDs that relate
      to a different history of the data set, and stil retain such IDs in the
      future while receiving unrelated writes.
      c1c99e9f
    • antirez's avatar
      PSYNC2: Create backlog on slave partial sync as well. · b75ae0bb
      antirez authored
      A slave may be started with an RDB file able to provide enough slave to
      perform a successful partial SYNC with its master. However in such a
      case, how outlined in issue #4268, the slave backlog will not be
      started, since it was only initialized on full syncs attempts. This
      creates different problems with successive PSYNC attempts that will
      always result in full synchronizations.
      
      Thanks to @fdingiit for discovering the issue.
      b75ae0bb
  16. 31 Aug, 2017 1 commit
  17. 27 Apr, 2017 1 commit
    • antirez's avatar
      PSYNC2: fix master cleanup when caching it. · 469d6e2b
      antirez authored
      The master client cleanup was incomplete: resetClient() was missing and
      the output buffer of the client was not reset, so pending commands
      related to the previous connection could be still sent.
      
      The first problem caused the client argument vector to be, at times,
      half populated, so that when the correct replication stream arrived the
      protcol got mixed to the arugments creating invalid commands that nobody
      called.
      
      Thanks to @yangsiran for also investigating this problem, after
      already providing important design / implementation hints for the
      original PSYNC2 issues (see referenced Github issue).
      
      Note that this commit adds a new function to the list library of Redis
      in order to be able to reset a list without destroying it.
      
      Related to issue #3899.
      469d6e2b
  18. 19 Apr, 2017 2 commits
    • antirez's avatar
      PSYNC2: discard pending transactions from cached master. · 189a12af
      antirez authored
      During the review of the fix for #3899, @yangsiran identified an
      implementation bug: given that the offset is now relative to the applied
      part of the replication log, when we cache a master, the successive
      PSYNC2 request will be made in order to *include* the transaction that
      was not completely processed. This means that we need to discard any
      pending transaction from our replication buffer: it will be re-executed.
      189a12af
    • antirez's avatar
      Fix PSYNC2 incomplete command bug as described in #3899. · 22be435e
      antirez authored
      This bug was discovered by @kevinmcgehee and constituted a major hidden
      bug in the PSYNC2 implementation, caused by the propagation from the
      master of incomplete commands to slaves.
      
      The bug had several results:
      
      1. Borrowing from Kevin text in the issue: "Given that slaves blindly
      copy over their master's input into their own replication backlog over
      successive read syscalls, it's possible that with large commands or
      small TCP buffers, partial commands are present in this buffer. If the
      master were to fail before successfully propagating the entire command
      to a slave, the slaves will never execute the partial command (since the
      client is invalidated) but will copy it to replication backlog which may
      relay those invalid bytes to its slaves on PSYNC2, corrupting the
      backlog and possibly other valid commands that follow the failover.
      Simple command boundaries aren't sufficient to capture this, either,
      because in the case of a MULTI/EXEC block, if the master successfully
      propagates a subset of the commands but not the EXEC, then the
      transaction in the backlog becomes corrupt and could corrupt other
      slaves that consume this data."
      
      2. As identified by @yangsiran later, there is another effect of the
      bug. For the same mechanism of the first problem, a slave having another
      slave, could receive a full resynchronization request with an already
      half-applied command in the backlog. Once the RDB is ready, it will be
      sent to the slave, and the replication will continue sending to the
      sub-slave the other half of the command, which is not valid.
      
      The fix, designed by @yangsiran and @antirez, and implemented by
      @antirez, uses a secondary buffer in order to feed the sub-masters and
      update the replication backlog and offsets, only when a given part of
      the query buffer is actually *applied* to the state of the instance,
      that is, when the command gets processed and the command is not pending
      in the Redis transaction buffer because of CLIENT_MULTI state.
      
      Given that now the backlog and offsets representation are in agreement
      with the actual processed commands, both issue 1 and 2 should no longer
      be possible.
      
      Thanks to @kevinmcgehee, @yangsiran and @oranagra for their work in
      identifying and designing a fix for this problem.
      22be435e
  19. 12 Apr, 2017 1 commit
  20. 20 Feb, 2017 1 commit
  21. 16 Dec, 2016 1 commit
    • antirez's avatar
      Hopefully improve code comments for issue #3616. · 8e390a62
      antirez authored
      This commit also contains other changes in order to conform the code to
      the Redis core style, specifically 80 chars max per line, smart
      conditionals in the same line:
      
          if (that) do_this();
      8e390a62
  22. 02 Dec, 2016 1 commit
  23. 29 Nov, 2016 1 commit
  24. 28 Nov, 2016 1 commit
    • antirez's avatar
      PSYNC2: stop sending newlines to sub-slaves when master is down. · eab865a0
      antirez authored
      This actually includes two changes:
      
      1) No newlines to take the master-slave link up when the upstream master
      is down. Doing this is dangerous because the sub-slave often is received
      replication protocol for an half-command, so can't receive newlines
      without desyncing the replication link, even with the code in order to
      cancel out the bytes that PSYNC2 was using. Moreover this is probably
      also not needed/sane, because anyway the slave can keep serving
      requests, and because if it's configured to don't serve stale data, it's
      a good idea, actually, to break the link.
      
      2) When a +CONTINUE with a different ID is received, we now break
      connection with the sub-slaves: they need to be notified as well. This
      was part of the original specification but for some reason it was not
      implemented in the code, and was alter found as a PSYNC2 bug in the
      integration testing.
      eab865a0
  25. 24 Nov, 2016 1 commit
  26. 23 Nov, 2016 1 commit
    • antirez's avatar
      PSYNC2: bugfixing pre release. · 5b7d42ff
      antirez authored
      1. Master replication offset was cleared after switching configuration
      to some other slave, since it was assumed you can't PSYNC after a
      switch. Note the case anymore and when we successfully PSYNC we need to
      have our offset untouched.
      
      2. Secondary replication ID was not reset to "000..." pattern at
      startup.
      
      3. Master in error state replying -LOADING or other transient errors
      forced the slave to discard the cached master and full resync. This is
      now fixed.
      
      4. Better logging of what's happening on failed PSYNCs.
      5b7d42ff
  27. 16 Nov, 2016 2 commits
  28. 10 Nov, 2016 1 commit
    • antirez's avatar
      PSYNC2: Save replication ID/offset on RDB file. · 28c96d73
      antirez authored
      This means that stopping a slave and restarting it will still make it
      able to PSYNC with the master. Moreover the master itself will retain
      its ID/offset, in case it gets turned into a slave, or if a slave will
      try to PSYNC with it with an exactly updated offset (otherwise there is
      no backlog).
      
      This change was possible thanks to PSYNC v2 that makes saving the current
      replication state much simpler.
      28c96d73
  29. 09 Nov, 2016 2 commits
    • antirez's avatar
      PSYNC2: Wrap debugging code with if(0) · 4e5e366e
      antirez authored
      4e5e366e
    • antirez's avatar
      PSYNC2: different improvements to Redis replication. · 2669fb83
      antirez authored
      The gist of the changes is that now, partial resynchronizations between
      slaves and masters (without the need of a full resync with RDB transfer
      and so forth), work in a number of cases when it was impossible
      in the past. For instance:
      
      1. When a slave is promoted to mastrer, the slaves of the old master can
      partially resynchronize with the new master.
      
      2. Chained slalves (slaves of slaves) can be moved to replicate to other
      slaves or the master itsef, without requiring a full resync.
      
      3. The master itself, after being turned into a slave, is able to
      partially resynchronize with the new master, when it joins replication
      again.
      
      In order to obtain this, the following main changes were operated:
      
      * Slaves also take a replication backlog, not just masters.
      
      * Same stream replication for all the slaves and sub slaves. The
      replication stream is identical from the top level master to its slaves
      and is also the same from the slaves to their sub-slaves and so forth.
      This means that if a slave is later promoted to master, it has the
      same replication backlong, and can partially resynchronize with its
      slaves (that were previously slaves of the old master).
      
      * A given replication history is no longer identified by the `runid` of
      a Redis node. There is instead a `replication ID` which changes every
      time the instance has a new history no longer coherent with the past
      one. So, for example, slaves publish the same replication history of
      their master, however when they are turned into masters, they publish
      a new replication ID, but still remember the old ID, so that they are
      able to partially resynchronize with slaves of the old master (up to a
      given offset).
      
      * The replication protocol was slightly modified so that a new extended
      +CONTINUE reply from the master is able to inform the slave of a
      replication ID change.
      
      * REPLCONF CAPA is used in order to notify masters that a slave is able
      to understand the new +CONTINUE reply.
      
      * The RDB file was extended with an auxiliary field that is able to
      select a given DB after loading in the slave, so that the slave can
      continue receiving the replication stream from the point it was
      disconnected without requiring the master to insert "SELECT" statements.
      This is useful in order to guarantee the "same stream" property, because
      the slave must be able to accumulate an identical backlog.
      
      * Slave pings to sub-slaves are now sent in a special form, when the
      top-level master is disconnected, in order to don't interfer with the
      replication stream. We just use out of band "\n" bytes as in other parts
      of the Redis protocol.
      
      An old design document is available here:
      
      https://gist.github.com/antirez/ae068f95c0d084891305
      
      However the implementation is not identical to the description because
      during the work to implement it, different changes were needed in order
      to make things working well.
      2669fb83