document that getset kills key expirations
mlissner opened this issue · 4 comments
(Sorry this is an issue and not a PR; I couldn't find where the getset documentation code is.)
I just learned today, over in freelawproject/courtlistener#1675 that getset nukes ttl. Here's some python code to demonstrate (r
is a connection to redis):
In [38]: r.set('asdf', "foo")
Out[38]: True
In [39]: r.expire('asdf', 60)
Out[39]: True
In [40]: r.ttl('asdf')
Out[40]: 55
In [41]: r.ttl('asdf')
Out[41]: 53
In [42]: r.getset('asdf', 'bar')
Out[42]: 'foo'
In [43]: r.ttl('asdf')
Out[43]: -1
I know getset is now deprecated, but this comes as a very big surprise to me, and I'm even more surprised it's not documented. I've noticed that SET
now has a KEEPTTL
parameter, so apparently this is something that has come up before.
Would it be a good idea to put a note about this in the getset
docs? Something like:
If a key already exists and has a time to live, the TTL will be reset.
I think that's all it would take and that might help others like me in the future.
Thank everybody for the wonderful thing that is redis. What a lovely tool.
Hello @mlissner - thanks for bringing this up.
I couldn't find where the getset documentation code is
https://github.com/redis/redis-doc/blob/master/commands/getset.md
I know getset is now deprecated
Indeed, it is deprecated in favor of the all-in-one evolved SET
command. That said, "deprecated" means very little here because I don't see us removing it from the project in the foreseeable future :)
so apparently this is something that has come up before
If you read the docs about SET
, you'll find that it resets TTL (and doesn't care about the TYPE
of the key if it exists). The KEEPTTL
option was added recently to modify this behavior. Because GETSET
also does a SET
, it is implied that it also resets the TTL.
Would it be a good idea to put a note about this
Sure. Please make the PR that clarifies this and close this issue.
Just created redis/redis-doc#1570 by copying from the SET documentation. Thanks for the guidance.
Looks like this small change to the docs got dropped. Any chance we can merge it?
Merged. Thanks @mlissner! This issue can be closed.