Backport more hfe fixes#3131
Closed
ranshid wants to merge 15 commits into
Closed
Conversation
…xiting fields (valkey-io#2973) When HEXPIRE commands are set with a time-in-the-past they are all deleting the specified fields. In such cases we allocate a temporal new argv in order to replicate `HDEL`. However in case no mutation was done (ie all fields do not exist) we do not deallocate the temporal new_argv and there is a memory leak. example: ``` HSET myhash field1 value1 1 HEXPIRE myhash 0 FIELDS 1 field2 -2 ``` --------- Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
Following Hash-Field-Expiration feature, a hash object can hold volatile fields. volatile fields which are already expired are deleted and reclaimed ONLY by the active-expiration background job. This means that hash object can contain items which have not yet expired. In case mutations are requesting to set a value on these "already-expired" fields, they will be overwritten with the new value. In such cases, though, it is requiered to update the global per-db tracking map by removing the key if it has no more volatile fields. This was implemented in all mutation cases of the hash commands but the `INCRBY` and `INCRBYFLOAT`. This can lead to a dangling object which has no volatile items, which might lead to assertion during the active-expiration job: example reproduction: ``` DEBUG SET-ACTIVE-EXPIRE 0 hset myhash f1 10 hexpire myhash 1 FIELDS 1 f1 sleep(10) hincrby myhash f1 1 DEBUG SET-ACTIVE-EXPIRE 1 ``` NOTE: we actually had tests for this scenario, only the test did not include explicit assertion in case the item is still tracked after the mutation. Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
If you send an `HSETEX key EXAT 0 FIELDS 1 foo bar`, it will create an empty length string. This is not good behavior as other places in the code will crash if an empty hash is accessed. This change just makes it so that the key is freed if it ends up being empty. This has some odd behavior w.r.t. to keyspace notifications though. Today, if you do `HSETEX key EXAT 0 FIELDS 1 foo bar` with a hash that exists, it won't send an `HSET` notification or an `HEXPIRE` notification unless foo is already present. It will also send a `DEL` notification if foo is last the field, effectively deleting the key. This isn't consistent with Redis, which will still send an `HSET` and `HDEL` (*NOTE* not `hexpire`) notification if a key is added past the expiration. Likewise, in the case where the key doesn't exist and `HSETEX key EXAT 0 FIELDS 1 foo bar` is sent, Redis will send an HSET, HDEL, and DEL notification in that order. I think it makes sense to keep consistency with what we have today (given that it's been out for awhile), but document the behavior. Basically, if you send a command with an expiration in the past, we will basically ignore the input unless it's "deleting" a key. Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
HSETEX right now uses `strcmp` which does not account for case-insensitivity replaced it with `strcasecmp`. Fixes valkey-io#2996 --------- Signed-off-by: frostzt <aidenfrostbite@gmail.com> Signed-off-by: Sourav Singh Rawat <aidenfrostbite@gmail.com> Co-authored-by: Ran Shidlansik <ranshid@amazon.com>
…alkey-io#3003) In `dbSetValue` if the robj is a hash with volatile fields we need to clean it up. It causes two core problems: 1. Type Confusion: Allows converting from hash to string with `keys_with_volatile_items` tracked 2. Tracked Items with dangling pointer: Converting from `hash` to ie. a string breaks this as `keys_with_volatile_items` is not cleaned up. Check valkey-io#3000 --------- Signed-off-by: frostzt <aidenfrostbite@gmail.com> Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
In `dbSetValue()` the `old` pointer may be reassigned to point to the incoming value object which was created without an embedded key, so calling `dbUntrackKeyWithVolatileItems()` would call `objectGetKey()` which returns NULL, causing a crash in `hashtableSdsHash()` when trying to hash the NULL key. Idea is to assign `old_was_hash_with_volatile` before the swap and use `new` instead of `old` for untracking when theres no embedded key. Introduced in valkey-io#3003 Run with NULL ptr dereference: https://github.com/valkey-io/valkey/actions/runs/20701343184/job/59424029880 --------- Signed-off-by: Daniil Kashapov <daniil.kashapov.ykt@gmail.com> Co-authored-by: Ran Shidlansik <ranshid@amazon.com> Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
…alkey-io#3022) if the hrandfield(e.g. hrandfield myhash) command without other args does not find a valid field, it will return an uninitialized lval. ``` debug set-active-expire no hsetex myhash ex 1 fields 2 f1 v1 f2 v2 after 1s... hrandfield myhash [will return some uninitialized number] ``` related: valkey-io#3021 Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
…on is in the past (valkey-io#3023) if an expired time is used, the condition is ignored, and it directly becomes the effect of the `hdel` command. This is mainly important for cases where the user would like to protect deleting the data in case the `HEXPIREAT` command is used and the user would like to protect delay execution of this command to delete fields. fixes: valkey-io#3021 --------- Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
…piration is in the past (valkey-io#3048) valkey-io#3023 was only partially solving the problem. We need to avoid expiring items in case of any validation rule failed. --------- Signed-off-by: Ran Shidlansik <ranshid@amazon.com> Co-authored-by: Binbin <binloveplay1314@qq.com> Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
…ey-io#3060) There are currently several issues with the existing hash field expiration mechanism: 1. `HINCRBY` is propagated to the replica "as-is". It mean it relies on the fact that the state of the hash is the same on the primary and the replica. HFE did change this assumption as the field might be expired only when the replica will handle the propagated `hincrby`. the problem is that the replica does not "expire" fields by it's own. it needs to respect the request from the primary and always try to use the existing field. This can lead to either miss-alignment with the value on the primary and the replica AND even a disconnection since the replica might hold and "expired" field which is not in "integer" format... 2. HINCRBYFLOAT is currently ALWAYS propagating `hset` - this means that the expiration time of an entry will always be removed on the replica side (it needs to propagate HSETEX when expiration time needs to be maintained) 3. Currently all hash write commands which are mutating values might overwrite an expired field. In such cases the existing implementation will "silently" do so. The problem is that the user will not get any key-space-notificaiton explaining the reason for the behavior. For example, when `hincrby` is issued overwriting an expired field which was not yet "cleaned" by active-expiration it will reset the counter to '0' before incrementing it. this means that the user might ask: why is the value '1' and not bigger, "I did not see any notification that the old value expired"... 4. HSETEX with KEEPTTL suffers from a "somewhat" similar problem as if the primary "replaced" the entry which is expired now but might not have been expired when the primary applied it. There are 2 options for a solution: 1. we could propagate `hdel` for every entry we are "overwritting" (batch them if we can) 2. propagate the commands "by effect". For example - have `hincrby` always propagate either HSET or HSETEX. This will not solve the '#(4)' problem above though, for which we might HAVE to propagate `hdel` I tend to go with the second option. The reason is that it is expected to have less impact on replication stream and should include less processing time on the replicas and network traffic. Specifically for HSETEX with KEEPTTL we will have to propagate the `hdel` in case we overwritten an expired field, but that would help limit the impact of this propagation. --------- Signed-off-by: Ran Shidlansik <ranshid@amazon.com> Co-authored-by: Sourav Singh Rawat <aidenfrostbite@gmail.com> Co-authored-by: Binbin <binloveplay1314@qq.com> Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
…alkey-io#3120) The test is currently flakey, because until we merge: valkey-io#3001 when the expiration time provided is in the past, and the field does not exist the HSETX will just silently ignore it, without incrementing the statistics. I prefer to focus on writing a dedicated test for: valkey-io#3001 and deflake this test now. Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
This caused some unaligned load of server values on 32bit compilations. Example: https://github.com/valkey-io/valkey/actions/runs/21352969942/job/61491923381?pr=3111#step:6:3064 Some insights here: valkey-io#3111 (comment) Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
…-io#3006) The HFE uses EXPIRY_NONE(-1) for fields without a TTL. A bug exists in `HSETEX` and RDB loading where `expiry > 0` is used to check for an expiration. This is problematic because `0` might be treated as no expiry in `import-mode`, instead of an already expired timestamp, leading to incorrect behavior. --------- Signed-off-by: cjx-zar <jxchenczar@foxmail.com>
…-io#3001) In the original implementation of Hash Field Expiration (valkey-io#2089), the HSETEX command was implemented to report keyspace notifications only for performed changes. This is mostly aligned with other Hash commands (for example, HDEL will also not report `hdel` event for items which does not exist) The HSETEX case is somewhat different and is more like the `HSET` case. During HSETEX, after the command validations pass, items are ALWAYS "added" to the object, even though they might not actually be added. This case is the same for when the hash object is empty or when all the provided fields do not exist in the object (as reported [here](valkey-io#2998)) This PR changes the way `HSETEX` will report keyspace notifications so that: 1. `hset` notification will ALWAYS be reported if all command validations pass. 2. `hexpire` will be reported in case the command include an expiration time (even past time) 3. `hxpired` will be reported in case the provided expiration time is in the past (or 0) 4. `hdel` will be reported in case the hash exists (or created as part of the command) and following the command execution it was left empty. 5. we will always return '1' as a return value of tHSETEX command which passed all validations. Before that we returned 1 only if we applied the change cross ALL the input fields, so in case some of them did not exist and a past time was set we would return 0. --------- Signed-off-by: Ran Shidlansik <ranshid@amazon.com> Co-authored-by: Jacob Murphy <jkmurphy@google.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 9.0 #3131 +/- ##
==========================================
+ Coverage 72.61% 72.65% +0.03%
==========================================
Files 128 128
Lines 69970 69972 +2
==========================================
+ Hits 50812 50835 +23
+ Misses 19158 19137 -21
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No release docs YET!