propagate changes for hsetex with PXAT absolute timestamps

[frostzt]

Created this so that you can actively review this @ranshid

[ranshid]

@frostzt PTAL. I changed the implementation foir hincr/hset/hsetnx to not propagate hdel. Still in our case ONLY when hsetex has the KEEPTTL flag, it will have to propagate. Also my changes around hsetex can be used to help your case

[frostzt]

@ranshid can you check once? I am now doing it in 3 stages:

1. If the field does not exist we use HDEL
2. If the field has KEEPTTL and has an expiry I send the command with PXAT
3. For field without expiry its just HSET

[ranshid]

@frostzt this is a big change you placed there :) I am not even sure why you need to go that far.
The way I see it is this:

1. We only need to collect the fields pointers (just the field name from the argument - c->argv[i]) into an array which could have been pre-allocated (you can also place it on the stack, but risk a stack overflow) according to the command number of fields (not fields+values)
2. What we need to do now s that in the place where I already placed the "hexpired" report:

else {
            if (expired_overriten) {
                server.stat_expiredfields += expired_overriten;
                notifyKeyspaceEvent(NOTIFY_HASH, "hexpired", c->argv[1], c->db->id);
            }

we need to run a loop calling propagateFieldsDeletion on the collected items and loop to the next collected items based on the value it returned. That is it :)

[frostzt]

@frostzt this is a big change you placed there :) I am not even sure why you need to go that far. The way I see it is this:

1. We only need to collect the fields pointers (just the field name from the argument - c->argv[i]) into an array which could have been pre-allocated (you can also place it on the stack, but risk a stack overflow) according to the command number of fields (not fields+values)

2. What we need to do now s that in the place where I already placed the "hexpired" report:

else {
            if (expired_overriten) {
                server.stat_expiredfields += expired_overriten;
                notifyKeyspaceEvent(NOTIFY_HASH, "hexpired", c->argv[1], c->db->id);
            }

we need to run a loop calling propagateFieldsDeletion on the collected items and loop to the next collected items based on the value it returned. That is it :)

Ah forgive me for that I kinda thought of it in a sequential way lemme update this, and thanks a lot for bearing with me 😭

[ranshid]

dropped some comments

[ranshid]

so keepttl_count and expired_overriten are always identical (if we decide to increment the expired_overriten whenever expired is set by hashTypeSet). I think we need not use 2 different values and we can simply use expired_overriten. I think the propagation logic should be applied here, but only in case keepttl_fields is not NULL.
The reason is that when KEEPTTL is provided the flag is set for ALL fields.

[frostzt]

done!

[ranshid]

@frostzt mostly LGTM. can you please add a test in hashexopire.tcl (you can do something in the inspiration of valkey-io#3036 and place it in the same place I placed all the other tests in valkey-io#3060

[frostzt]

@ranshid I think I messed up here! Caught it while running tests locally I increased expired_overriten however we're using that as index which is causing the server to panic, I think a simple post increase should be good?

[ranshid]

Yeh it should be increased after we use it to index the position

[ranshid]

@frostzt 2 issues:

1. in order to support the KEEPTTL we need to fix how hashTypeSet function manages expirations.
   This means that instead of doing:

is_expired = timestampIsExpired(entry_expiry);

we should do:

is_expired = entry_expiry != EXPIRY_NONE && checkAlreadyExpired(entry_expiry);

this is in order to make sure replicas/import mode and slot migration are operating correctly by forcing the logic instructed by the replication/import/migration stream
I will make this change in my PR since it will also require some of my tests to change.

2. The test is not really testing what we wanted.
   We have 2 issues with KEEPTTL:
   a. the case of a primary HSETEX overwrites an expired field which was not yet reclaimed by the ActiveExpiration
   b. the case of primary HSETEX overwrites an NOT expired field BUT when the command reaches the replica it will use the old way (as explained in #(1) - timestampIsExpired) and thus will do something different than what the primary was doing.
   The propagation of the hdel which you are working on, is required to solve case 'a' AFTER we solve case 'b' :)
   I mean the fix in #(1) will introduce a problem in the case of 'a' which is why we will propagate hdel in order to make sure the replica will not KEPPTTL of a field which the primary ALOS expired by overwrting it.

I suggest 2 tests for each of the cases a/b:

 test {HSETEX KEEPTTL replica should preserve ttl when field is not expired on primary} {
            lassign [setup_replication_test $primary $replica $primary_host $primary_port] primary_initial_expired replica_initial_expired
            $primary debug set-active-expire 0

            $primary hset myhash f1 v1

            wait_for_ofs_sync $primary $replica

            pause_process $replica_pid
            
            $primary multi
            $primary hpexpire myhash 1 fields 1 f1
            $primary hsetex myhash KEEPTTL fields 1 f1 v2
            $primary exec

            # wait for f1 to expired
            wait_for_condition 50 100 {
                [$primary httl myhash fields 1 f1] == -2
            } else {
                fail "Field was not logically expired on primary"
            }

            resume_process $replica_pid

            wait_for_ofs_sync $primary $replica

            assert_equal {-2} [$primary httl myhash fields 1 f1]
            assert_equal {-2} [$replica httl myhash fields 1 f1]
            $primary debug set-active-expire 1
        } {OK} {needs:debug}

        test {HSETEX KEEPTTL replica should NOT preserve ttl when field is expired on primary} {
            lassign [setup_replication_test $primary $replica $primary_host $primary_port] primary_initial_expired replica_initial_expired
            $primary debug set-active-expire 0

            # write a short lived field on the primary and wait for the propagation
            $primary hsetex myhash PX 1 fields 1 f1 v1
        
            # wait for f1 to expired
            wait_for_condition 50 100 {
                [$primary httl myhash fields 1 f1] == -2
            } else {
                fail "Field was not logically expired on primary"
            }

            # Now overite the expired field on the primary and wait for it to propagate to the replica
            $primary hsetex myhash KEEPTTL fields 1 f1 v2
            wait_for_ofs_sync $primary $replica

            assert_equal {v2} [$primary hget myhash f1]
            assert_equal {v2} [$replica hget myhash f1]
            $primary debug set-active-expire 1
        } {OK} {needs:debug}

[ranshid]

@frostzt for time constraints I made some of the changes I suggested in my PR. please merge it to yours and focus on the new hdel propagation.
Currently in my latest changes the test HSETEX KEEPTTL replica should NOT preserve ttl when field is expired on primary will fail. and your fix is the one that should make it work again

[frostzt]

@frostzt for time constraints I made some of the changes I suggested in my PR. please merge it to yours and focus on the new hdel propagation. Currently in my latest changes the test HSETEX KEEPTTL replica should NOT preserve ttl when field is expired on primary will fail. and your fix is the one that should make it work again

Thanks @ranshid just took a pull!
Question: With the above by doing this

is_expired = entry_expiry != EXPIRY_NONE && checkAlreadyExpired(entry_expiry);

This essentially is for fields that don't define a TTL. My question now is for hdel propagations this would impact the above that I have written right? What should my logical flow for tackling this be?

[ranshid]

we also rewrite the argv when we have the FXX/FNX and NX/XX flags, so we cannot "else if" here

[frostzt]

ok this fixed the test, @ranshid if you don't mind (I'll push the changes) but can you explain this to me a little more?

[ranshid]

more about what?

[ranshid]

@frostzt for time constraints I made some of the changes I suggested in my PR. please merge it to yours and focus on the new hdel propagation. Currently in my latest changes the test HSETEX KEEPTTL replica should NOT preserve ttl when field is expired on primary will fail. and your fix is the one that should make it work again

Thanks @ranshid just took a pull! Question: With the above by doing this

is_expired = entry_expiry != EXPIRY_NONE && checkAlreadyExpired(entry_expiry);

This essentially is for fields that don't define a TTL. My question now is for hdel propagations this would impact the above that I have written right? What should my logical flow for tackling this be?

Your currently implemented logic is fine. In hsetex when we got an indication that we overwritten an expired field and KEEPTTL is used (ONLY THEN) we should ALSO propagate hdel the command itself will be propagated right after

[ranshid]

this will mask the next 2 lines which SHOULD be executed even if keepttl_fields is null. keepttl_fields is not NULL only when we overwritten expired fields AND KEEPTTL flag was provided. however even if KEEPTTL was not provided, we might have overwritten some fields which were found to be expired

[frostzt]

oh snap yeah, that makes sense

[frostzt]

I think I just attached my thinking that overritten == overritten with KEEPTTL

[ranshid]

no, so this is why I split the effort. the overwriting of expired keys is a general problem/bug I fixed in my PR. the specific case of KEEPTTL is another issue

[ranshid]

Thank you sooooo much @frostzt !

[ranshid]

still not sure why this is needed?

[frostzt]

oh checking the code for propagateFieldDeletion I get it my bad again I didn't knew that it does that already!

[ranshid]

yes, the ref count ownership is passed to this function.
I suggest always compile and test with ASAN:
make -j noopt SANITIZER=address OPTIMIZATION=-O0

[ranshid]

Great @frostzt I will now merge it and take it from here! much much Thank you!

[frostzt]

Thank you sooooo much @frostzt !

I honestly don't know if I deserve that but all thanks to you I really wanted to work in Valkey and have been just mesmerizing its code I can assure you the effort you put in to help me means a lot honestly I learned so much I'll keep up and keep contributing into this and hopefully will be able to write much better PRs (that will be easier for you to review 😛 )