Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Don't expire keys when loading an RDB after a SYNC #296

Merged
merged 1 commit into from

3 participants

Pieter Noordhuis Mark Smith Salvatore Sanfilippo
Pieter Noordhuis
Collaborator

A user reported a slave that would show an monotonically increase input buffer length, shortly after completing a SYNC. Also, INFO output showed a single blocked client, which could only be the master link. Investigation showed that indeed the BRPOP command was fed by the master. This command can only end up in the stream of write operations when it did NOT block, and effectively executed RPOP. However, when the key involved in the BRPOP is expired BEFORE the command is executed, the client executing it will block. The client in this case, is the master link.

Pieter Noordhuis pietern Don't expire keys when loading an RDB after a SYNC
The cron is responsible for expiring keys. When keys are expired at
load time, it is possible that the snapshot of a master node gets
modified. This can in turn lead to inconsistencies in the data set.

A more concrete example of this behavior follows. A user reported a
slave that would show an monotonically increase input buffer length,
shortly after completing a SYNC. Also, `INFO` output showed a single
blocked client, which could only be the master link. Investigation
showed that indeed the `BRPOP` command was fed by the master. This
command can only end up in the stream of write operations when it did
NOT block, and effectively executed `RPOP`. However, when the key
involved in the `BRPOP` is expired BEFORE the command is executed, the
client executing it will block. The client in this case, is the master
link.
aa794ac
Pieter Noordhuis
Collaborator

/cc @xb95

Pieter Noordhuis
Collaborator

I was not able to reproduce the issue because of timing issues. However, this is very likely to fix the issue.

Mark Smith

I have deployed this patch on our slave and it is now synced and replicating as expected. Thank you!

Salvatore Sanfilippo
Owner

This is a good one, the slave should never expire by itself. To reason about the changes introduced by this patch, now that the slave is not expiring at loading time the master still accumulates the synthesized DELs on expire server side, so those keys will be correctly handled. However it is strange that the replication test with expires was never able to catch this bug.

I'm merging but I'm taking this issue open to merge it into 2.4 and in the hope to write a regression test for it, or at least, to modify the current replication with expires test so that it is likely that we can catch it even 1 time every 1000 runs (the CI will make it consistent).

Thank you.

Salvatore Sanfilippo antirez merged commit 58bfbd1 into from
Jackie JackieXie168 referenced this pull request from a commit
Daniel Mewes danielmewes Clean up throttling code. Also reset default unsaved data limit confi…
…guration to values close to v_0.1.x.

Closes #296.
452eb8f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 14, 2012
  1. Pieter Noordhuis

    Don't expire keys when loading an RDB after a SYNC

    pietern authored
    The cron is responsible for expiring keys. When keys are expired at
    load time, it is possible that the snapshot of a master node gets
    modified. This can in turn lead to inconsistencies in the data set.
    
    A more concrete example of this behavior follows. A user reported a
    slave that would show an monotonically increase input buffer length,
    shortly after completing a SYNC. Also, `INFO` output showed a single
    blocked client, which could only be the master link. Investigation
    showed that indeed the `BRPOP` command was fed by the master. This
    command can only end up in the stream of write operations when it did
    NOT block, and effectively executed `RPOP`. However, when the key
    involved in the `BRPOP` is expired BEFORE the command is executed, the
    client executing it will block. The client in this case, is the master
    link.
This page is out of date. Refresh to see the latest.
Showing with 6 additions and 2 deletions.
  1. +6 −2 src/rdb.c
8 src/rdb.c
View
@@ -993,8 +993,12 @@ int rdbLoad(char *filename) {
if ((key = rdbLoadStringObject(fp)) == NULL) goto eoferr;
/* Read value */
if ((val = rdbLoadObject(type,fp)) == NULL) goto eoferr;
- /* Check if the key already expired */
- if (expiretime != -1 && expiretime < now) {
+ /* Check if the key already expired. This function is used when loading
+ * an RDB file from disk, either at startup, or when an RDB was
+ * received from the master. In the latter case, the master is
+ * responsible for key expiry. If we would expire keys here, the
+ * snapshot taken by the master may not be reflected on the slave. */
+ if (server.masterhost == NULL && expiretime != -1 && expiretime < now) {
decrRefCount(key);
decrRefCount(val);
continue;
Something went wrong with that request. Please try again.