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

Add SETNE #4258

Open
wants to merge 1 commit into
base: unstable
Choose a base branch
from
Open

Add SETNE #4258

wants to merge 1 commit into from

Conversation

erikdubbelboer
Copy link
Contributor

Add command to only set a value when it's different than the current value. This is useful when listening for changes and you don't want to be notified when a value doesn't change.

This native command is about 3 times faster than using a lua script and is only a few new lines.

See: https://www.reddit.com/r/redis/comments/6vry8x/proposed_new_command_setne_set_if_not_equal/

Add command to only set a value when it's different than the current
value. This is useful when listening for changes and you don't want to
be notified when a value doesn't change.
@masterclock
Copy link

what's the progress of this PR?

@slayerulan
Copy link

Any update regarding this PR?

@GeoffreyPlitt
Copy link

+1 Really want this.

@kristoff-it
Copy link
Contributor

Hi, I quickly wrote an implementation of this command as a module.
You can find the source code here:
https://github.com/kristoff-it/redis-setne

If you don't know / care about Zig, just download a pre-compiled dynamic library from the Releases section.

@kinkey
Copy link

kinkey commented Apr 13, 2020

What is the status of this PR? Thanks in advance.

@erikdubbelboer
Copy link
Contributor Author

I could rebase it on master if there was a chance of it being merged. Buf after more thant 2 years I doubt it will be.

@yoav-steinberg yoav-steinberg added state:needs-investigation the problem or solution is not clear, some investigation is needed and removed state:needs-investigation the problem or solution is not clear, some investigation is needed labels Dec 16, 2021
@yoav-steinberg
Copy link
Contributor

yoav-steinberg commented Dec 16, 2021

The pros here are mostly performance related since there's an easy solution using scripting.
The cons are that this might be too specific for string set command and scripting can be used.
@itamarhaber @oranagra WDYT?
In any case I suggest we handle this or close it since it's not a big issue and the PR is ready.

@itamarhaber
Copy link
Member

I think if this is added, instead of a new string command, SETs syntax can be added with an NE flag.

I don't think we should add a new flag though. If a SET doesn't modify the data, did it really happen? Assuming nothing's changed (i.e. also TTL is the same), should an event be generated or even a replication entry for that matter?

@madolson
Copy link
Contributor

Another interesting case that comes to mind is client side caching, it would be an optimization to not invalidate a key if the value didn't change.

Idk though, LUA seems like the right approach here to me, this seems close to CAS which we've also been hesitant to implement. I suppose I would vote to close this, but don't feel strongly.

@yoav-steinberg
Copy link
Contributor

yoav-steinberg commented Dec 19, 2021

I don't think we should add a new flag though. If a SET doesn't modify the data, did it really happen? Assuming nothing's changed (i.e. also TTL is the same), should an event be generated or even a replication entry for that matter?

The problem with treating this as just a SET optimization is:

  • This might be considered a breaking change since in the past we did send notifications on this "nop".
  • There's a serious performance issue here: doing the extra memcmp before each memcpy will definitely make SET slower.

@madolson I'm marking to be closed, as I think this can be treated as a special case of SETCAS (#8361) and we should continue discussion there. I'll reference this ticket there.

@yoav-steinberg yoav-steinberg added the state:to-be-closed requesting the core team to close the issue label Dec 19, 2021
@motz0815
Copy link

The problem with treating this as just a SET optimization is:

* This might be considered a breaking change since in the past we did send notifications on this "nop".

* There's a serious performance issue here: doing the extra `memcmp` before each `memcpy` will definitely make `SET` slower.

@madolson I'm marking to be closed, as I think this can be treated as a special case of SETCAS (#8361) and we should continue discussion there. I'll reference this ticket there.

Wouldn't that be even slower than just adding a new command that does that all by itself?

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:to-be-closed requesting the core team to close the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet