diff --git a/src/rdb.c b/src/rdb.c index b9006a49869..c7584420f42 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -2237,7 +2237,7 @@ robj *rdbLoadObject(int rdbtype, rio *rdb, sds key, int dbid, int *error) { return NULL; } - if (rdbtype == RDB_TYPE_HASH_2 && itemexpiry > 0) { + if (rdbtype == RDB_TYPE_HASH_2 && itemexpiry != EXPIRY_NONE) { hashTypeTrackEntry(o, entry); } } diff --git a/src/t_hash.c b/src/t_hash.c index 7a941d2a030..486de9a7a6f 100644 --- a/src/t_hash.c +++ b/src/t_hash.c @@ -335,7 +335,7 @@ int hashTypeSet(robj *o, sds field, sds value, long long expiry, int flags, bool * This is needed for HINCRBY* case since in other commands this is handled early by * hashTypeTryConversion, so this check will be a NOP. */ if (o->encoding == OBJ_ENCODING_LISTPACK) { - if (expiry > 0 || sdslen(field) > server.hash_max_listpack_value || sdslen(value) > server.hash_max_listpack_value) + if (expiry != EXPIRY_NONE || sdslen(field) > server.hash_max_listpack_value || sdslen(value) > server.hash_max_listpack_value) hashTypeConvert(o, OBJ_ENCODING_HASHTABLE); } @@ -1326,6 +1326,7 @@ void hsetexCommand(client *c) { return; if (checkAlreadyExpired(when)) { + need_rewrite_argv = 1; set_expired = 1; } } @@ -1396,13 +1397,15 @@ void hsetexCommand(client *c) { for (i = fields_index; i < c->argc; i += 2) { if (set_expired) { + hashTypeIgnoreTTL(o, true); if (hashTypeDelete(o, c->argv[i]->ptr)) { new_argv[new_argc++] = c->argv[i]; incrRefCount(c->argv[i]); - /* we treat this case exactly as active expiration. */ - server.stat_expiredfields++; changes++; } + /* we treat this case exactly as active expiration. */ + server.stat_expiredfields++; + hashTypeIgnoreTTL(o, false); } else { bool expired; hashTypeSet(o, c->argv[i]->ptr, c->argv[i + 1]->ptr, when, set_flags, &expired); @@ -1435,40 +1438,31 @@ void hsetexCommand(client *c) { if (has_volatile_fields != hashTypeHasVolatileFields(o)) { dbUpdateObjectWithVolatileItemsTracking(c->db, o); } - if (set_expired) { - replaceClientCommandVector(c, new_argc, new_argv); - /* We would like to reduce the number of hexpired events in case there are potential many expired fields. */ + + /* In case we overwritten fields which were expired we need to act as if we actively expired them */ + if (expired_overwritten > 0) { + server.stat_expiredfields += expired_overwritten; notifyKeyspaceEvent(NOTIFY_HASH, "hexpired", c->argv[1], c->db->id); - } else { - /* In case we overwritten fields which were expired we need to act as if we actively expired them */ - if (expired_overwritten > 0) { - server.stat_expiredfields += expired_overwritten; - notifyKeyspaceEvent(NOTIFY_HASH, "hexpired", c->argv[1], c->db->id); - /* Propagate deletions for expired/non-existent fields in batches. - * When KEEPTTL is used the replica has noway telling if, at the time the primary was executing the command, - * the fields were expired or not. When the replica executes the command it will ALWAYS overwrite the field, so - * we need to propagate hdel explicitly to prevent the replica from keeping the TTL on it's side. */ - if (keepttl_fields != NULL) { - /* Propagate individual fields deletions */ - int idx = 0; - while (idx < expired_overwritten) { - idx += propagateFieldsDeletion(c->db, o, expired_overwritten - idx, - &keepttl_fields[idx], c->slot); - } - zfree(keepttl_fields); - keepttl_fields = NULL; + /* Propagate deletions for expired/non-existent fields in batches. + * When KEEPTTL is used the replica has noway telling if, at the time the primary was executing the command, + * the fields were expired or not. When the replica executes the command it will ALWAYS overwrite the field, so + * we need to propagate hdel explicitly to prevent the replica from keeping the TTL on it's side. */ + if (keepttl_fields != NULL) { + /* Propagate individual fields deletions */ + int idx = 0; + while (idx < expired_overwritten) { + idx += propagateFieldsDeletion(c->db, o, expired_overwritten - idx, + &keepttl_fields[idx], c->slot); } + zfree(keepttl_fields); + keepttl_fields = NULL; } + } - notifyKeyspaceEvent(NOTIFY_HASH, "hset", c->argv[1], c->db->id); - - if (need_rewrite_argv) { - replaceClientCommandVector(c, new_argc, new_argv); - } - if (expire) { - notifyKeyspaceEvent(NOTIFY_HASH, "hexpire", c->argv[1], c->db->id); - } + if (need_rewrite_argv) { + replaceClientCommandVector(c, new_argc, new_argv); } + signalModifiedKey(c, c->db, c->argv[1]); server.dirty += changes; } else { @@ -1478,6 +1472,17 @@ void hsetexCommand(client *c) { if (new_argv) zfree(new_argv); } + /* Handle keyspace notifications and object delete if needed. + * since setting fields in hash object should always work in case all validations pass, + * it is safe to assume that in case we reach this point events should be issues */ + notifyKeyspaceEvent(NOTIFY_HASH, "hset", c->argv[1], c->db->id); + if (expire) { + notifyKeyspaceEvent(NOTIFY_HASH, "hexpire", c->argv[1], c->db->id); + } + if (set_expired) { + /* We would like to reduce the number of hexpired events in case there are potential many expired fields. */ + notifyKeyspaceEvent(NOTIFY_HASH, "hexpired", c->argv[1], c->db->id); + } /* Delete the object in case it was left empty or created with all expired items. */ if (hashTypeLength(o) == 0) { dbDelete(c->db, c->argv[1]); @@ -1485,7 +1490,9 @@ void hsetexCommand(client *c) { } /* make sure that if we ever allocated this it was freed */ serverAssert(keepttl_fields == NULL); - addReplyLongLong(c, changes == num_fields ? 1 : 0); + /* In case we reached here we know that we operated on ALL the fields, + * even in case we end up in the same original state, we still need to reflect as the operation was done on all the fields. */ + addReply(c, shared.cone); } /* High-Level Algorithm of HGETEX Command: diff --git a/tests/unit/hashexpire.tcl b/tests/unit/hashexpire.tcl index f11299cf7a7..639ed998dad 100644 --- a/tests/unit/hashexpire.tcl +++ b/tests/unit/hashexpire.tcl @@ -555,7 +555,7 @@ start_server {tags {"hashexpire"}} { } {ERR *} foreach command {EX PX EXAT PXAT} { - test "HSETEX $command 0/past time works correctly with 1 field" { + test "HSETEX $command 0/past time works correctly with 2 fields" { r FLUSHALL r config resetstat # Create hash with field @@ -566,16 +566,16 @@ start_server {tags {"hashexpire"}} { set rd [setup_single_keyspace_notification r] # Set field to expire immediately - assert_equal {1} [r HSETEX myhash $command [get_past_zero_expire_value $command] FIELDS 1 f1 v1] + assert_equal {1} [r HSETEX myhash $command [get_past_zero_expire_value $command] FIELDS 2 f1 v1 f2 v2] # Verify field and keys are deleted - assert_keyevent_patterns $rd myhash hexpired del + assert_keyevent_patterns $rd myhash hset hexpire hexpired del assert_equal -2 [r HTTL myhash FIELDS 1 f1] assert_equal 0 [r HLEN myhash] assert_equal 0 [r EXISTS myhash] assert_equal 0 [get_keys r] assert_equal 0 [get_keys_with_volatile_items r] - assert_equal 1 [info_field [r info stats] expired_fields] + assert_equal 2 [info_field [r info stats] expired_fields] $rd close } } @@ -632,7 +632,6 @@ start_server {tags {"hashexpire"}} { assert_error {ERR numfields should be greater than 0 and match the provided number of fields} {r HSETEX myhash PX 100 FIELDS 1 field1 val1 extra} } - ## FNX/FXX # hsetex throws ERR *, it shouldn't @@ -835,7 +834,7 @@ start_server {tags {"hashexpire"}} { r FLUSHALL r HSET myhash f1 v1 set rd [setup_single_keyspace_notification r] - + r HEXPIRE myhash 1000 FIELDS 1 f2 r HEXPIRE myhash 0 FIELDS 1 f2 # Verify no notification (getting hset and not hexpire) @@ -844,7 +843,7 @@ start_server {tags {"hashexpire"}} { assert_equal 0 [get_keys_with_volatile_items r] $rd close } - + # Error Cases test {HEXPIRE - conflicting conditions error} { r FLUSHALL @@ -4662,4 +4661,23 @@ start_server {tags {"hash"}} { assert_match {*keys_with_volatile_items=0*} [r INFO keyspace] assert_equal {string} [r TYPE myhash] } + + test {Zero is a valid ttl in HFE} { + r flushall + r hset myhash f1 v1 + assert_equal [r OBJECT ENCODING myhash] "listpack" + assert_equal [r hsetex myhash exat 0 fields 2 f2 v2 f3 v3] 1 + assert_equal [r hlen myhash] 1 + assert_equal [r OBJECT ENCODING myhash] "listpack" + r config set import-mode yes + assert_equal [r hsetex myhash exat 0 fields 2 f2 v2 f3 v3] 1 + assert_equal [r hlen myhash] 3 + assert_equal [r OBJECT ENCODING myhash] "hashtable" + r config set import-mode no + wait_for_condition 30 100 { + [r hlen myhash] == 1 + } else { + fail "field wasn't expired" + } + } }