Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the correct command proc for the LOOKUP_NOTOUCH exception in lookupKey #1499

Merged
merged 1 commit into from
Jan 3, 2025

Conversation

gmbnomis
Copy link
Contributor

When looking up a key in no-touch mode, LOOKUP_NOTOUCH is set to avoid updating the last access time in lookupKey. An exception must be made for the TOUCH command which must always update the key.

When called from a script, server.executing_client will point to the TOUCH command, while server.current_client will point to e.g. an EVAL command. So, we must use the former to find out the currently executing command if defined.

This fix addresses the issue where TOUCH wasn't updating key access times when called from scripts like EVAL.

Fixes #1498

@gmbnomis
Copy link
Contributor Author

To avoid any licensing issues, I did not look into the corresponding fix in Redis. I can provide a back-port to Valkey 7.2 if desired.

Copy link

codecov bot commented Dec 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.79%. Comparing base (ae70c54) to head (b29a5f8).
Report is 5 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1499      +/-   ##
============================================
- Coverage     70.84%   70.79%   -0.06%     
============================================
  Files           119      120       +1     
  Lines         64880    64911      +31     
============================================
- Hits          45967    45956      -11     
- Misses        18913    18955      +42     
Files with missing lines Coverage Δ
src/db.c 89.55% <100.00%> (ø)

... and 13 files with indirect coverage changes

src/db.c Outdated Show resolved Hide resolved
src/db.c Outdated Show resolved Hide resolved
@enjoy-binbin enjoy-binbin added the release-notes This issue should get a line item in the release notes label Jan 2, 2025
@gmbnomis gmbnomis force-pushed the fix_touch_in_script branch from 40cbc65 to 4cfa6fb Compare January 2, 2025 14:18
@hwware
Copy link
Member

hwware commented Jan 2, 2025

Once CI pass, we can merge it.

…upKey

When looking up a key in no-touch mode, `LOOKUP_NOTOUCH` is set to avoid
updating the last access time in `lookupKey`. An exception must be made
for the `TOUCH` command which must always update the key.

When called from a script, `server.executing_client` will point to the
`TOUCH` command, while `server.current_client` will point to e.g. an
`EVAL` command. So, we must use the former to find out the currently
executing command.

This fix addresses the issue where TOUCH wasn't updating
key access times when called from scripts like EVAL.

Fixes valkey-io#1498

Signed-off-by: Simon Baatz <[email protected]>
@gmbnomis gmbnomis force-pushed the fix_touch_in_script branch from 4cfa6fb to b29a5f8 Compare January 3, 2025 00:30
@gmbnomis
Copy link
Contributor Author

gmbnomis commented Jan 3, 2025

Once CI pass, we can merge it.

@hwware I rebased to the current unstable branch and the CI is green now

@enjoy-binbin enjoy-binbin merged commit 26a72fa into valkey-io:unstable Jan 3, 2025
50 checks passed
@enjoy-binbin
Copy link
Member

thanks for the fix, merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes This issue should get a line item in the release notes
Projects
Status: Merged
Status: To be backported
Development

Successfully merging this pull request may close these issues.

[BUG] TOUCH has no effect in scripts when client is in no-touch mode
3 participants