Describe the bug

When executing HINCRBYFLOAT on a field of HFE, the expiration time of that field is removed on the replica, even though the master retains it. This leads to an inconsistency between master and replica.

To reproduce

  • master
127.0.0.1:6379> hset hash f1 1
(integer) 1
127.0.0.1:6379> hexpire hash 200 fields 1 f1
1) (integer) 1
127.0.0.1:6379> hincrbyfloat hash f1 1
"2"
127.0.0.1:6379> HTTL hash fields 1 f1
1) (integer) 187
  • replica
127.0.0.1:6380> httl hash fields 1 f1  <== after hexpire
1) (integer) 197
127.0.0.1:6380> httl hash fields 1 f1 <== after hincrbyfloat
1) (integer) -1

Expected behavior

Field expiration metadata should be preserved on the replica after HINCRBYFLOAT

Additional information

the root cause is that we rewrite HINCRBYFLOAT to HSET after executing HINCRBYFLOAT

https://github.com/redis/redis/blob/f6f16746e1d4bc51960158d9a896e1aa0a2c7dbd/src/t_hash.c#L2565-L2572

maybe we can use HSETEX ... KEEPTTL, but HSETEX is introduced from 8.0, the bug should be introduced from 7.4

Comment From: ShooterIT

could you please have a look @moticless

Comment From: moticless

    /* Always replicate HINCRBYFLOAT as an HSET command with the final value
     * in order to make sure that differences in float precision or formatting
     * will not create differences in replicas or after an AOF restart. */

Maybe for 8.0 we can use HSETEX ... KEEPTTL And for 7.4 we can hack it with if no expiration time then keep behaviour. Otherwise, use HINCRBYFLOAT for replica and prioritize having TTL over float precision in that case. @oranagra WDYT?

Comment From: ShooterIT

OK, let's fix for 8.0+ first

Comment From: tezc

can't we replicate as multiple commands inside MULTI for 7.4?

Comment From: oranagra

for 8.0 we should use KEEPTTL. for 7.0, we can keep using HSET when we don't have a ttl. and when we do, we can either fallback to propagate HINCRBYFLOAT (sacrificing that precision concern), or replicate two commands (HSET and EXPIRE) which will implicitly get wrapped by MULTI

Comment From: ShooterIT

If we can call alsoPropagate(hexpireat) at the end of HINCRBYFLOAT command, so the replicated commands are hexpireat + hset, instead of hset + hexpireat, the order is not expected, do i miss something? i don't want to have a big change for 7.4 version.

Comment From: tezc

@ShooterIT I think you are right. It'll be in reverse order.

Perhaps we can do something like:

preventCommandPropagation()
alsoPropagate(hset)
alsoPropagate(hexpireat)

Comment From: ShooterIT

I didn't notice preventCommandPropagation, great, will try, thank you @tezc

Comment From: oranagra

i think what matters here is not how many lines the change has, but it's implications. i.e if we have conditions, and as long as the field doesn't have expiration, we keep propagating as in 7.0, that means that the change is smaller.

Comment From: ShooterIT

if we have conditions, and as long as the field doesn't have expiration, we keep propagating as in 7.0

sure, we should do that

as you said, if a field has expiration, there are two methods to resolve this issue - hset + hexpireat - replicate hincrbyfloat without changing command

i am worried that replicating hincrbyfloat directly may cause a break change, so i want to try to hset + hexpireat

before @tezc reminding, i think it is not easy to keep the order hset + hexpireat, we can not call alsoPropagate(hexpireat) at the end of hincrbyfloatCommand directly.

Comment From: oranagra

you can call preventCommandPropagation, like SPOP does