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

Extended SET command #931

Closed
antirez opened this issue Feb 4, 2013 · 29 comments
Closed

Extended SET command #931

antirez opened this issue Feb 4, 2013 · 29 comments
Assignees

Comments

@antirez
Copy link
Contributor

antirez commented Feb 4, 2013

SET and variants

SET is one of the most used Redis commands, and has many variants:

  • SETNX -- Set if not exists
  • SETEX -- Set with expire
  • PSETEX -- Set with milliseconds expire

There is demand for additional variants such as SETEXNX, becuase it is a pretty common pattern in caching and sessions handling to set an object only if not already inside the cache, and setting it with an expire. This is also useful for poor's man locking, and so forth.

There is the argument that this need can be addressed with scripting, however probably this is enough common that a very high performance solution is expected, and also we can exploit the need in order to make our SET story more clear for the users.

Trivia: the reason currently we have this mess of commands is because of an early limitation of the Redis protocol in the early stages. The binary-safe argument was only possible as last argument, so it was not easy to extend already existing commands, and I kept introducing new ones. Finally the protocol was fixed and now our hands are free.

Proposal

This proposal extends the SET command to cover all the already existing variants in form of options, so that later all the SET variants will be deprecated.
At the same time the proposal adds a new functionality in form of an option that only executes the SET command if the key already exists.

The new form of the command will be:

SET key value option option option ...

The available options will be:

  • nx -- Execute the SET only if the key does not already exists
  • xx -- Execute the SET only if the key already exists
  • ex -- Set an expire if the SET will be executed.
  • px -- Set a milliseconds expire if the SET will be executed.

Examples:

SET foo bar xx # Set foo to bar only if key foo already exists.
SET foo value ex 1000 nx # Set foo to value with an expire of 1000 seconds, if not exists.
SET foo bar ex 10 # Same as SETEX foo 10 bar

The new command will allow more variants in the future if needed and is simple to remember. The option names were chosen to be short to write and easy to remember just remembering the first character:

  • Non existing
  • eXisting
  • Eexpire
  • Pprecision expire

Feedbacks welcomed.

@ghost ghost assigned antirez Feb 4, 2013
@mgravell
Copy link

mgravell commented Feb 4, 2013

I think it would be a positive change, and would make taking locks a breeze - just:

set {lockname} {lockvalue} nx ex {timeout}

My concern, however, would be the return value and the impact on older clients; set has historically returned +ok; however now it would presumably be taking on the return signature of setnx, i.e. :0 or :1. This could negatively impact older clients that are checking for a +ok response, with false-negative failures when using regular set.

Unless, of course, the intent is to return +ok if nx / xx are not used, and :0 / :1 if nx / xx are used.

Thoughts?

@antirez
Copy link
Contributor Author

antirez commented Feb 4, 2013

Good point Marc, my reasoning was to always return just :0 / :1 actually because in the vanilla form SET can't fail, so probably nobody checks the return value, but yep if there is an explicit check, this will break existing clients. There is to evaluate if this is the case or not.

The backward compatible way of returning OK with no options works for sure, but given that we are going for fixing it, maybe it's better to try hard to return a consistent return value. However there is also an alternative of returning "+OK" if the command works as expected and "+NOOP" or alike in case nx or xx prevented the command from being executed. In that case we have a single return value, it is backward compatible, but it introduces something new to know for the users.

Ok there is to think a bit about this :-) thanks for the hint!

@melo
Copy link
Contributor

melo commented Feb 4, 2013

@mgravell I don't see the return value as an issue.

If you plan to use the new command, you will probably check the client library to see if already supports it or not, and update it if not.

As long as the old SET version in the new code returns the same old +OK return code, old code (code that uses an old client lib) will only use the current syntax, and will keep returning the current response code.

@antirez Do we need EX and PX? Why not always precision expire? Also, why the short codes (EX instead of EXPIRE)?

@mgravell
Copy link

mgravell commented Feb 4, 2013

@melo it is the same command; the proposal (see also antirez's comment) would potentially change the result of the existing set command; there is no "new command" here. Old clients, continuing to use the command that they always have, but against a new server version, may suddenly get a different response.

@antirez
Copy link
Contributor Author

antirez commented Feb 4, 2013

@melo unfortunately the new version would NOT return OK even in the old form... But :1

About ex/px, given that we have options, it is probably not too bad to also allow people to use seconds that is what 99% of people do with expires, so my reason is to add two options solely based on the frequency of usage.

About the short options, two reasons:

    1. fast to type.
    1. fast to check in the code, no string comparison but inline character checks.

About point 2, because the initial character is different for every option so far, checking this will be very very fast just with a switch or alike for the first char, and only later we'll check the rest of the string, that must just be 'x' + nullterm.

@mgravell
Copy link

mgravell commented Feb 4, 2013

@antirez "set can't fail" is incorrect; it might be true in the "happy path", but it can fail in many other ways:

  • when reading the db file / synchronizing ("not ready" iirc)
  • with "READONLY You can't write against a read only slave."
  • and possibly a few others

What I have not done is look at all the available client-library implementations to see which disregard the result completely; which just check for -message (for error conditions), and which positively check for +ok.

I've checked my own library, and I have an ExpectOk() there; obviously that is totally trivial for me to fix; my point there is : it is not beyond reason that other clients have their equivalent.

@antirez
Copy link
Contributor Author

antirez commented Feb 4, 2013

True but in that case a -ERR exception will be raised exactly like in the new SET case, that's why I don't think people check for the 'OK' string itself but for exceptions (errors).

@mgravell
Copy link

mgravell commented Feb 4, 2013

I chose another library at random and looked at the "set", and indeed in that library it just checked for -message failures. I guess it comes down to the confidence level of libraries being overly conservative in what they accept.

@melo
Copy link
Contributor

melo commented Feb 4, 2013

I may be missing something obvious, but I see no reason that the new code must return a different result from the one we have today if the simple SET k v format is used.

Only if options are used does a new return code makes sense, IMVHO.

@antirez
Copy link
Contributor Author

antirez commented Feb 4, 2013

@mgravell yep I think we'll break almost nothing at all in the practice, and anyway this is for 2.8 so we'll make sure to put a big warning in the release notes :-)

@melo I want to have a single return value because often people do something like that:

cmd = ... code to dynamically generate a set ...
result = redis.set(*cmd)
if result == 0 # We were unable to set the key
   .... some other code ...
end

Basically in complex code, often inside library clients implementation, you don't know the form it is used, and to check explicitly if the form is a "pure" one or not will make the check more complex.

@dragonsinth
Copy link

This seems awesome, love the proposal. Don't have any brilliant suggestions on the return value tho.

@antirez
Copy link
Contributor Author

antirez commented Feb 4, 2013

Dudes, quick poll on Twitter revealed with this change we'll break a lot of code.

Currently the best option we have as an exit strategy is +OK on success, +NOOP on no op performed.
But +NOOP kind of sucks, a better more clear to understand string would be much better.

@antirez
Copy link
Contributor Author

antirez commented Feb 5, 2013

I reconsidered the return value issue, basically OK / NOOP will not help us further, and will create an asymmetry as all the other commands use the :1 / :0 pattern.

The reason why it does not help is that in the initial time after 2.8 introduction clients will not be always updated, so people will check for explicit NOOP returned by the command.
Later probably most clients will update to return true/false to allow for the if (redis.set(...)) pattern, and this will break the code checking for NOOP.

So probably to return :0 and :1 is our best bet, with information about the change.

Similarly I would like to rename HMSET to HSET (variadic version by default) and deprecate HMSET.

So practically speaking, what this will break?

  • Application code that explicitly tests for OK returned by set.
  • Client libraries that try to ensure SET returns OK
  • Client libraries that process the reply from SET in a way that a status reply is expected and an integer reply can't be handled properly.

In some way or the other we should try to move forward and fix the API by providing changes and deprecating the old way of doing things. There isn't a lot to fix in the Redis API, but this time I don't think there are better options.

@e-oz
Copy link

e-oz commented Feb 5, 2013

In languages with dynamic typing it doesn't matter, will this function return +OK or 1. if (redis.set(... will be possible and correct anyway. But not with +NOOP. Dynamic typed languages are widely used in web-dev, so I guess it will not be a big problem.

For compatibility it will be easy to create wrappers for old variants of SET in clients. Because client library can't afford to declare support only one (last) version of Redis :)

@antirez
Copy link
Contributor Author

antirez commented Feb 5, 2013

@Jamm For both Ruby and Lua for instance numerical 0 is a true value... so I'm not sure if it's really that different compared to NOOP.

@e-oz
Copy link

e-oz commented Feb 5, 2013

Sorry, but +NOOP looks ugly.

@e-oz
Copy link

e-oz commented Feb 5, 2013

If 1/0 is a generic pattern, then Ruby and Lua clients should just reflect it and take as true/false... I think it will be more predictable behavior, than one more special value as NOOP.

@robreiss
Copy link

robreiss commented Feb 5, 2013

I don't have much experience with Redis yet, just brainstorming:

Can we put the options at the front?
SET -e 60 key1 val1
SET -ye 60 key1 val1 key2 val2 ...
SET -n key1 val1

y = yes exists, n = not exists, e = expire, p = millisecond expire

I could see this being nice when generating a variadic SET via a loop or something, ie. don't have to tag on the options after the loop.

@aleksraiden
Copy link

robreiss - i think, your example hasnt backward compartability and protocol/command style of other command at Redis, IMHO

@arnodirlam
Copy link

Great proposal, I like the idea of making the Redis API more consistent and flexible at the same time.

How about adding two more options:

  • eq -- Execute the SET only if the existing value equals given parameter (after eq)
  • ne -- Execute the SET only if the existing value does not equal given parameter (after ne)

Of course this would require comparing both characters, not just the first one, but there are might be sensible use cases for these options, e.g. setting a value only if it hasn't been overwritten in the meantime with one atomic operation (kind of an optimistic lock). What do you guys think?

@charsyam
Copy link
Contributor

charsyam commented Feb 7, 2013

How about adding new command just follow above rule
for example "setx", as you know, setnx and set have different response and, it can cause many problem.

so I think, supporting new command, as time goes by, we can deprecated setnx and other thing.

@ptaoussanis
Copy link

My vote:

  • I really like this proposal. Very happy to see there's still thought going into improving the basics when possible.
  • I like the idea of returning :0/:1. I think a documented, sensible break would be better than the +NOOP (which will still leave unnecessary difficulties in the future). It's one clean break, for a good benefit.
  • I also like @arnodirlam's suggestion of also adding something like eq, ne since these are themselves such common/useful cases.

@antirez What about also offering a config option (disabled by default) that retains the old SET semantics for those few clients that do break and haven't yet been updated?

@grosser
Copy link

grosser commented Mar 26, 2013

👍 please add this or add setexnx, we need a 100% reliable way of locking.

@antirez
Copy link
Contributor Author

antirez commented Mar 28, 2013

Dudes after much considering all this issue, my guess is that we are best served by the following return value:

  • If the set was performed, return +OK, no breakage with the past.
  • If the set was not performed, return a null bulk reply like EXEC does with WATCH when the transaction is aborted.

So this is perfectly compatible with the past, and also should work without issues using the new format even with clients not explicitly accounting for the new format because the null bulk reply is often returned as "nil" that most languages will evaluate as a false value.

And here the bottom line is, with this return values making it backward-compatible 100% with the old SET semantics, we can marge in current 2.6 branch and have it ASAP. I think it's a good idea after all. Comments welcomed.

@mgravell
Copy link

That's a fair solution - zero impact on clients that don't use the new extended api, so no surprises. And some nice single-trip conditional operations. Very cute.

@antirez
Copy link
Contributor Author

antirez commented Mar 28, 2013

Thanks Marc, the implementation is now available in the unstable branch with tests. Everything seems fine, so I'm planning to backport this into 2.6 ASAP (before 2.6.12).

@antirez
Copy link
Contributor Author

antirez commented Apr 2, 2013

Implemented, back ported into 2.6, 2.6.12 released, and documented at redis.io. Closing :-)

Thank you for all your feedbacks!

@antirez antirez closed this as completed Apr 2, 2013
@mgravell
Copy link

mgravell commented Apr 3, 2013

The locking guidance on http://redis.io/commands/setnx should probably be updated with this in mind. The most obvious lock now is simply:

set {key} {token} [ex|px] {timeout} nx

with the check: if you get +OK, you got the lock; if you get *0, you didn't.

Edit: I see this is mentioned in "set", but IMO the "setnx" page should strongly suggest using the "set" version, only using the "setnx" implementation for down-level servers.

@antirez
Copy link
Contributor Author

antirez commented Apr 3, 2013

@mgravell agreed, doing it right now. I'm also writing the small Lua script required to safely unlock for reference so that I can add it into the SET page. Thanks.

pombredanne pushed a commit to pombredanne/redis-py that referenced this issue Nov 2, 2013
JackieXie168 pushed a commit to JackieXie168/redis that referenced this issue Aug 29, 2016
yossigo added a commit to yossigo/redis that referenced this issue May 30, 2023
b6a052fe0 Helper for setting TCP_USER_TIMEOUT socket option (redis#1188)
3fa9b6944 Add RedisModule adapter (redis#1182)
d13c091e9 Fix wincrypt symbols conflict
5d84c8cfd Add a test ensuring we don't clobber connection error.
3f95fcdae Don't attempt to set a timeout if we are in an error state.
aacb84b8d Fix typo in makefile.
563b062e3 Accept -nan per the RESP3 spec recommendation.
04c1b5b02 Fix colliding option values
4ca8e73f6 Rework searching for openssl
cd208812f Attempt to find the correct path for openssl.
011f7093c Allow specifying the keepalive interval
e9243d4f7 Cmake static or shared (redis#1160)
1cbd5bc76 Write a version file for the CMake package (redis#1165)
6f5bae8c6 fix typo
acd09461d CMakeLists.txt: respect BUILD_SHARED_LIBS
97fcf0fd1 Add sdevent adapter
ccff093bc Bump dev version for the next release cycle.
c14775b4e Prepare for v1.1.0 GA
f0bdf8405 Add support for nan in RESP3 double (redis#1133)
991b0b0b3 Add an example that calls redisCommandArgv (redis#1140)
a36686f84 CI updates (redis#1139)
8ad4985e9 fix flag reference
7583ebb1b Make freeing a NULL redisAsyncContext a no op.
2c53dea7f Update version in dev branch.
f063370ed Prepare for v1.1.0-rc1
2b069573a CI fixes in preparation of release
e1e9eb40d Add author information to release-drafter template.
afc29ee1a Update for mingw cross compile
ceb8a8815 fixed cpp build error with adapters/libhv.h
3b15a04b5 Fixup of PR734: Coverage of hiredis.c (redis#1124)
c245df9fb CMake corrections for building on Windows (redis#1122)
9c338a598 Fix PUSH handler tests for Redis >= 7.0.5
6d5c3ee74 Install on windows fixes (redis#1117)
68b29e1ad Add timeout support to libhv adapter. (redis#1109)
722e3409c Additional include directory given by pkg-config (redis#1118)
bd9ccb8c4 Use __attribute__ when building with clang on windows
5392adc26 set default SSL certificate directory
560e66486 Minor refactor
d756f68a5 Add libhv example to our standard Makefile
a66916719 Add adapters/libhv
855b48a81 Fix pkgconfig for hiredis_ssl
79ae5ffc6 Fix protocol error (redis#1106)
61b5b299f Use a windows specific keepalive function. (redis#1104)
fce8abc1c Introduce .close method for redisContextFuncs
cfb6ca881 Add REDIS_OPT_PREFER_UNSPEC (redis#1101)
cc7c35ce6 Update documentation to explain redisConnectWithOptions.
bc8d837b7 fix heap-buffer-overflow (redis#957)
ca4a0e850 uvadapter: reduce number of uv_poll_start calls
35d398c90 Fix cmake config path on Linux. CMake config files were installed to `/usr/local/share/hiredis`, which is not recognizable by `find_package()`. I'm not sure why it was set that way. Given the commit introducing it is for Windows, I keep that behavior consistent there, but fix the rest.
10c78c6e1 Add possibility to prefer IPv6, IPv4 or unspecified
1abe0c828 fuzzer: No alloc in redisFormatCommand() when fail
329eaf9ba Fix heap-buffer-overflow issue in redisvFormatCommad
eaae7321c Polling adapter requires sockcompat.h
0a5fa3dde Regression test for off-by-one parsing error
9e174e8f7 Add do while(0) protection for macros
4ad99c69a Rework asSleep to be a generic millisleep function.
75cb6c1ea Do store command timeout in the context for redisSetTimeout (redis#593)
c57cad658 CMake: remove dict.c form hiredis_sources
8491a65a9 Add Github Actions CI workflow for hiredis: Arm, Arm64, 386, windows. (redis#943)
77e4f09ea Merge pull request redis#964 from afcidk/fix-createDoubleObject
9219f7e7c Merge pull request redis#901 from devnexen/illumos_test_fix
810cc6104 Merge pull request redis#905 from sundb/master
df8b74d69 Merge pull request redis#1091 from redis/ssl-error-ub-fix
0ed6cdec3 Fix some undefined behaviour
507a6dcaa Merge pull request redis#1090 from Nordix/subscribe-oom-error
b044eaa6a Copy error to redisAsyncContext when finding subscribe cb
e0200b797 Merge pull request redis#1087 from redis/const-and-non-const-callback
6a3e96ad2 Maintain backward compatibiliy withour onConnect callback.
e7afd998f Merge pull request redis#1079 from SukkaW/drop-macos-10.15-runner
17c8fe079 Merge pull request redis#931 from kristjanvalur/pr2
b808c0c20 Merge pull request redis#1083 from chayim/ck-drafter
367a82bf0 Merge pull request redis#1085 from stanhu/ssl-improve-options-setting
71119a71d Make it possible to set SSL verify mode
dd7979ac1 Merge pull request redis#1084 from stanhu/sh-improve-ssl-docs
c71116178 Improve example for SSL initialization in README.md
5c9b6b571 Release drafter
a606ccf2a CI: use recommended `vmactions/freebsd-vm@v0`
0865c115b Merge pull request redis#1080 from Nordix/readme-corrections
f6cee7142 Fix README typos
06be7ff31 Merge pull request redis#1050 from smmir-cent/fix-cmake-version
7dd833d54 CI: bump macos runner version
f69fac769 Drop `const` on redisAsyncContext in redisConnectCallback Since the callback is now re-entrant, it can call apis such as redisAsyncDisconnect()
005d7edeb Support calling redisAsyncDisconnect from the onConnected callback, by deferring context deletion
6ed060920 Add async regression test for issue redis#931
eaa2a7ee7 Merge pull request redis#932 from kristjanvalur/pr3
2ccef30f3 Add regression test for issue redis#945
4b901d44a Initial async tests
31c91408e Polling adapter and example
8a15f4d65 Merge pull request redis#1057 from orgads/static-name
902dd047f Merge pull request redis#1054 from kristjanvalur/pr08
c78d0926b Merge pull request redis#1074 from michael-grunder/kristjanvalur-pr4
2b115d56c Whitespace
1343988ce Fix typos
47b57aa24 Add some documentation on connect/disconnect callbacks and command callbacks
a890d9ce2 Merge pull request redis#1073 from michael-grunder/kristjanvalur-pr1
f246ee433 Whitespace, style
94c1985bd Use correct type for getsockopt()
5e002bc21 Support failed async connects on windows.
5d68ad2f4 Merge pull request redis#1072 from michael-grunder/fix-redis7-unit-tests
f4b6ed289 Fix tests so they work for Redis 7.0
95a0c1283 Merge pull request redis#1058 from orgads/win64
eedb37a65 Fix warnings on Win64
47c3ecefc Merge pull request redis#1062 from yossigo/fix-push-notification-order
e23d91c97 Merge pull request redis#1061 from yossigo/update-redis-apt
34211ad54 Merge pull request redis#1063 from redis/fix-windows-tests
9957af7e3 Whitelist hiredis repo path in cygwin
b455b3381 Handle push notifications before or after reply.
aed9ce446 Use official repository for redis package.
d7683f35a Merge pull request redis#1047 from Nordix/unsubscribe-handling
7c44a9d7e Merge pull request redis#1045 from Nordix/sds-updates
dd4bf9783 Use the same name for static and shared libraries
ff57c18b9 Embed debug information in windows static lib, rather than create a .pdb file
8310ad4f5 fix cmake version
7123b87f6 Handle any pipelined unsubscribe in async
b6fb548fc Ignore pubsub replies without a channel/pattern
00b82683b Handle overflows as errors instead of asserting
64062a1d4 Catch size_t overflows in sds.c
066c6de79 Use size_t/long to avoid truncation
c6657ef65 Merge branch 'redis:master' into master
50cdcab49 Fix potential fault at createDoubleObject
fd033e983 Remove semicolon after do-while in _EL_CLEANUP
664c415e7 Illumos test fixes, error message difference fot bad hostname test.

git-subtree-dir: deps/hiredis
git-subtree-split: b6a052fe0959dae69e16b9d74449faeb1b70dbe1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests