HSETEX: Support NX/XX Flags #2668

hanxizh9910 · 2025-09-30T22:23:50Z

Summary

Addresses #2619.
This PR extends the HSETEX command to support optional key-level NX and XX flags, allowing operations conditional on the existence of the hash key.

Changes

Updated hsetex.json and regenerated commands.def.
Extended argument parsing for NX/XX.
Added key-level NX/XX support in HSETEX.
Added tests covering all four NX/XX scenarios.

codecov · 2025-09-30T22:42:45Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.47%. Comparing base (d29d580) to head (c1673e1).
⚠️ Report is 23 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #2668      +/-   ##
============================================
- Coverage     72.57%   72.47%   -0.10%     
============================================
  Files           128      128              
  Lines         71277    70171    -1106     
============================================
- Hits          51727    50857     -870     
+ Misses        19550    19314     -236

Files with missing lines	Coverage Δ
src/commands.def	`100.00% <ø> (ø)`
src/server.c	`88.41% <ø> (-0.02%)`	⬇️
src/t_hash.c	`96.16% <100.00%> (-0.09%)`	⬇️

... and 102 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ranshid · 2025-10-01T05:30:47Z

Thank you @hanxizh9910 ! I am starting a holiday so will try to start review next week (sorry for that)

src/commands/hsetex.json

src/t_hash.c

ranshid

left some small test comments.

One thing that I just thought of is the forward compatibility. Since we are adding this in 9.1 it will make it problematic to sync commands withy the new NX/XX arguments as it is persisted on the replication stream.
Before that we kept the FXX/FNX for simplicity, but I think that we should now rewrite the command vector in case either FXX/FNX/NX/XX are included to prevent some weird replication misalignments and support replicating to 9.0.

@zuiderkwast FYI

tests/unit/hashexpire.tcl

zuiderkwast · 2025-10-08T10:27:08Z

Before that we kept the FXX/FNX for simplicity, but I think that we should now rewrite the command vector in case either FXX/FNX/NX/XX are included to prevent some weird replication misalignments and support replicating to 9.0.

Technically, NX and XX are new parameters in 9.1, so by starting to use them, the user opts in to the new feature. Using a new feature in the new version means that the possibility to downgrade ends. With this reasoning, it should be OK to just add them and replicate them. That said, I think it's cleaner to rewrite the command and replicate without them.

ranshid · 2025-10-08T13:46:56Z

Before that we kept the FXX/FNX for simplicity, but I think that we should now rewrite the command vector in case either FXX/FNX/NX/XX are included to prevent some weird replication misalignments and support replicating to 9.0.

Technically, NX and XX are new parameters in 9.1, so by starting to use them, the user opts in to the new feature. Using a new feature in the new version means that the possibility to downgrade ends. With this reasoning, it should be OK to just add them and replicate them. That said, I think it's cleaner to rewrite the command and replicate without them.

Agree, but I think we should take extra care to try and avoid breaking this in minor version.

ranshid · 2025-10-10T06:55:57Z

@hanxizh9910 thank you for spotting the bug in HSETEX with FXX!

However I suspect we can fix that in the upcoming 9.0 GA (it is a trivial fix IMO) and it will prevent leaving an empty key which can cause trouble.
Let me drive a separate PR and you can rebase this in case it will be taken to 9.0

ranshid · 2025-10-10T07:31:55Z

here: #2716

Signed-off-by: Hanxi Zhang <hanxizh@amazon.com> Generated new commands.def Signed-off-by: Hanxi Zhang <hanxizh@amazon.com> Extended parseExtendedCommandArgumentsOrReply function to handle nx and xx for COMMAND_HSET Signed-off-by: Hanxi Zhang <hanxizh@amazon.com> Added logic in hsetexCommand function to handle NX/XX Signed-off-by: Hanxi Zhang <hanxizh@amazon.com> Fix: removed two spaces to pass the format test' Signed-off-by: Hanxi Zhang <hanxizh@amazon.com> Added tests for nx/xx in hsetex command Signed-off-by: Hanxi Zhang <hanxizh@amazon.com>

Signed-off-by: Hanxi Zhang <hanxizh@amazon.com>

hanxizh9910 · 2025-10-11T00:10:58Z

@hanxizh9910 thank you for spotting the bug in HSETEX with FXX!

However I suspect we can fix that in the upcoming 9.0 GA (it is a trivial fix IMO) and it will prevent leaving an empty key which can cause trouble. Let me drive a separate PR and you can rebase this in case it will be taken to 9.0

@ranshid , thank you for your feedback! I got the changes you made!

hanxizh9910 · 2025-10-11T00:14:43Z

Hi @ranshid, I made a commit to attempt rewriting the command. Could you please let me know if this is what you had in mind. Also, do you want this rewrite change included in this PR? Thank you!

ranshid · 2025-10-15T08:43:11Z

src/t_hash.c

+        /* Rewrite command vector for replication */
+        if (flags & (ARGS_SET_NX | ARGS_SET_XX | ARGS_SET_FNX | ARGS_SET_FXX)) {
+            int rewrite_argc = 2 + num_fields * 2; // command, hash + field/value pairs
+            robj **rewrite_argv = zmalloc(sizeof(robj *) * rewrite_argc);
+            int j = 0;
+
+            // First argument: command
+            rewrite_argv[j++] = c->argv[0];
+            incrRefCount(c->argv[0]);
+
+            // Second argument: hash
+            rewrite_argv[j++] = c->argv[1];
+            incrRefCount(c->argv[1]);
+
+            // Copy all field/value pairs, skipping NX/XX/FNX/FXX flags
+            for (int i = fields_index; i < c->argc; i++) {
+                // Skip NX/XX/FNX/FXX flags
+                if (strcmp(c->argv[i]->ptr, "NX") &&
+                    strcmp(c->argv[i]->ptr, "XX") &&
+                    strcmp(c->argv[i]->ptr, "FNX") &&
+                    strcmp(c->argv[i]->ptr, "FXX")) {
+                    rewrite_argv[j++] = c->argv[i];
+                    incrRefCount(c->argv[i]);
+                }
+            }
+
+            // Replace client command vector
+            replaceClientCommandVector(c, j, rewrite_argv);
+        }
+


I think this might work, but it is somewhat wasteful. For example, when we are expiring elements (eg expiration time is zero) we already managing a separate argv. I think maybe it would be better to simply use and manage a new_argv when we are doing the hashTypeSet. We can optimize by not adding the parameters when the new_argv is NULL.

Hi @ranshid, thanks for the feedback! I’ve updated the function to use new_argv. Is this what you had in mind?

Hi @ranshid, thanks for the feedback! I’ve updated the function to use new_argv. Is this what you had in mind?

Thank you @hanxizh9910.
Not exactly what I meant is that we might refactor the function to:

we can set an early mark that we need to use a new argv in case flags & (ARGS_SET_NX | ARGS_SET_XX | ARGS_SET_FNX | ARGS_SET_FXX))

Then we can allocate the relevant new_argv as an else case for the if condition in line 1263, like:

if (set_expired) { new_argv = zmalloc(sizeof(robj *) * (num_fields + 2)); new_argv[new_argc++] = shared.hdel; incrRefCount(shared.hdel); new_argv[new_argc++] = c->argv[1]; incrRefCount(c->argv[1]); } else if (flags & (ARGS_SET_NX | ARGS_SET_XX | ARGS_SET_FNX | ARGS_SET_FXX)) ) { new_argv = zmalloc(sizeof(robj *) * (num_fields + 2)); .... }

We should extend the code in line 1281 to manage the new_argv:

} else { hashTypeSet(o, c->argv[i]->ptr, c->argv[i + 1]->ptr, when, set_flags); changes++; if (flags & (ARGS_SET_NX | ARGS_SET_XX | ARGS_SET_FNX | ARGS_SET_FXX))) { new_argv[new_argc++] = c->argv[i]; incrRefCount(c->argv[i]); } }

rewrite the command vector after we notify the keyspace event in line 1296

This might need to also handle the PXAT conversion differently (maybe earlier?) but that was my suggestion.

For your suggestion:

this if should probably be placed under the previous else in line 1225, since when set_expired is true, we already rewrite the command differently without these values.

there is no need to compare each field but only fields in index 2 and up to fields_index

I guess we could also use your suggestion as well (with the 2 fixes I listed before).

Hi @ranshid, thank you for your suggestion! I’ve modified the code accordingly, does this match what you had in mind?

Also, I have a question about the PXAT conversion: if no fields are modified due to NX/XX/FNX/FXX, should we still update the expiration time?

Signed-off-by: Hanxi Zhang <hanxizh@amazon.com>

…w_argc, new_argv) incorrectly before Signed-off-by: Hanxi Zhang <hanxizh@amazon.com>

Signed-off-by: Hanxi Zhang <hanxizh@amazon.com>

github-actions bot assigned hanxizh9910 Sep 30, 2025

sarthakaggarwal97 requested a review from ranshid September 30, 2025 22:41

madolson added this to Valkey 9.1 Oct 1, 2025

madolson moved this to Todo in Valkey 9.1 Oct 1, 2025

ranshid reviewed Oct 5, 2025

View reviewed changes

src/commands/hsetex.json Outdated Show resolved Hide resolved

src/t_hash.c Show resolved Hide resolved

ranshid reviewed Oct 8, 2025

View reviewed changes

tests/unit/hashexpire.tcl Show resolved Hide resolved

tests/unit/hashexpire.tcl Show resolved Hide resolved

hanxizh9910 added 6 commits October 10, 2025 18:32

Combine 2 checks into a single if statment

3506718

Signed-off-by: Hanxi Zhang <hanxizh@amazon.com>

Added parenthesis to the if statement

a73cbf8

Signed-off-by: Hanxi Zhang <hanxizh@amazon.com>

Separate NX/XX key checks from FNX/FXX field checks in HSETEX

f7761d7

Signed-off-by: Hanxi Zhang <hanxizh@amazon.com>

Added check for the keys in the tests

cc68158

Signed-off-by: Hanxi Zhang <hanxizh@amazon.com>

Fix: fix the issue when FNX/FXX failed but the key was still created

d8bcc1f

Signed-off-by: Hanxi Zhang <hanxizh@amazon.com>

hanxizh9910 force-pushed the feature/hsetex-nx-xx-support branch from 48584bb to d8bcc1f Compare October 10, 2025 18:36

hanxizh9910 added 2 commits October 10, 2025 23:57

hsetex: try rewriting command vector to skip NX/XX/FNX/FXX

56fc42f

Signed-off-by: Hanxi Zhang <hanxizh@amazon.com>

Fix the clang-format issue

ef7bb0c

Signed-off-by: Hanxi Zhang <hanxizh@amazon.com>

ranshid reviewed Oct 15, 2025

View reviewed changes

hanxizh9910 added 6 commits October 17, 2025 01:12

Refactor: using new_argv for rewrite

ae5ddda

Signed-off-by: Hanxi Zhang <hanxizh@amazon.com>

Fix: fixed a logic error, placed the replaceClientCommandVector(c, ne…

5d228f8

…w_argc, new_argv) incorrectly before Signed-off-by: Hanxi Zhang <hanxizh@amazon.com>

Added NX/XX/FNX/FXX based on Reviewer's suggestion

9d69a8c

Signed-off-by: Hanxi Zhang <hanxizh@amazon.com>

Refactored the new_argv initialization

97e7646

Signed-off-by: Hanxi Zhang <hanxizh@amazon.com>

Fix: added if check when rewrite the command

90b54f9

Signed-off-by: Hanxi Zhang <hanxizh@amazon.com>

Fix a clang format issue

c1673e1

Signed-off-by: Hanxi Zhang <hanxizh@amazon.com>

Uh oh!

HSETEX: Support NX/XX Flags #2668

Are you sure you want to change the base?

HSETEX: Support NX/XX Flags #2668

Conversation

hanxizh9910 commented Sep 30, 2025

Summary

Changes

Uh oh!

codecov bot commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ranshid commented Oct 1, 2025

Uh oh!

Uh oh!

Uh oh!

ranshid left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zuiderkwast commented Oct 8, 2025

Uh oh!

ranshid commented Oct 8, 2025

Uh oh!

ranshid commented Oct 10, 2025

Uh oh!

ranshid commented Oct 10, 2025

Uh oh!

hanxizh9910 commented Oct 11, 2025

Uh oh!

hanxizh9910 commented Oct 11, 2025

Uh oh!

ranshid Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

hanxizh9910 Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

ranshid Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

hanxizh9910 Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Sep 30, 2025 •

edited

Loading

ranshid left a comment •

edited

Loading

hanxizh9910 Oct 24, 2025 •

edited

Loading