Skip to content

Conversation

oranagra
Copy link
Member

@oranagra oranagra commented Jan 16, 2022

The new ACL key based permissions in #9974 require the key-specs (#8324) to have more explicit flags rather than just READ and WRITE. See discussion in #10040

This PR defines two groups of flags:
One about how redis internally handles the key (mutually-exclusive).
The other is about the logical operation done from the user's point of view (3 mutually exclusive write flags, and one read flag, all optional).
In both groups, if we can't explicitly flag something as explicit read-only, delete-only, or insert-only, we flag it as RW or UPDATE.
here's the definition from the code:

/* Key-spec flags *
 * -------------- */
/* The following refer what the command actually does with the value or metadata
 * of the key, and not necessarily the user data or how it affects it.
 * Each key-spec may must have exaclty one of these. Any operation that's not
 * distinctly deletion, overwrite or read-only would be marked as RW. */
#define CMD_KEY_RO (1ULL<<0)     /* Read-Only - Reads the value of the key, but
                                  * doesn't necessarily returns it. */
#define CMD_KEY_RW (1ULL<<1)     /* Read-Write - Modifies the data stored in the
                                  * value of the key or its metadata. */
#define CMD_KEY_OW (1ULL<<2)     /* Overwrite - Overwrites the data stored in
                                  * the value of the key. */
#define CMD_KEY_RM (1ULL<<3)     /* Deletes the key. */
/* The follwing refer to user data inside the value of the key, not the metadata
 * like LRU, type, cardinality. It refers to the logical operation on the user's
 * data (actual input strings / TTL), being used / returned / copied / changed,
 * It doesn't refer to modification or returning of metadata (like type, count,
 * presence of data). Any write that's not INSERT or DELETE, would be an UPADTE.
 * Each key-spec may have one of the writes with or without access, or none: */
#define CMD_KEY_ACCESS (1ULL<<4) /* Returns, copies or uses the user data from
                                  * the value of the key. */
#define CMD_KEY_UPDATE (1ULL<<5) /* Updates data to the value, new value may
                                  * depend on the old value. */
#define CMD_KEY_INSERT (1ULL<<6) /* Adds data to the value with no chance of,
                                  * modification or deletion of existing data. */
#define CMD_KEY_DELETE (1ULL<<7) /* Explicitly deletes some content
                                  * from the value of the key. */

Unrelated changes:

  • generate-command-code.py is only compatible with python3 (modified the shabang)
  • generate-command-code.py print file on json parsing error
  • rename shard_channel key-spec flag to just channel.
  • add INCOMPLETE flag in input spec of SORT and SORT_RO

@oranagra
Copy link
Member Author

oranagra commented Jan 16, 2022

spec.csv.gz

Spec RO RW OW RM ACCESS UPDATE INSERT DELETE
HMGET x x
GEODIST x x
XREVRANGE x x
DECR x x x
PERSIST x x
PEXPIREAT x x
LCS x x
HSTRLEN x
LTRIM x x
SORT 0 x x
SORT 1 x x
SUNION x x
ZSCAN x x
XGROUP DELCONSUMER x x
XREADGROUP x x
XGROUP DESTROY x x
MSETNX x x
SPOP x x x
XADD x x
ZDIFF x x
HVALS x x
ZINTERCARD x
SMEMBERS x x
PFMERGE 0 x x x
PFMERGE 1 x x
ZADD x x
LLEN x
OBJECT FREQ x
SSCAN x x
RPOPLPUSH 0 x x x
RPOPLPUSH 1 x x
XINFO CONSUMERS x x
SSUBSCRIBE
HMSET x x
XACK x x
BZMPOP x x x
XSETID x x
XTRIM x x
GETBIT x x
GEOADD x x
HSET x x
SADD x x
APPEND x x
EXPIREAT x x
RENAMENX 0 x x x
RENAMENX 1 x x
ZINTER x x
EVALSHA_RO x x
RENAME 0 x x x
RENAME 1 x x
SINTER x x
LSET x x
ZCOUNT x
ZREMRANGEBYRANK x x
ZRANGESTORE 0 x x
ZRANGESTORE 1 x x
SDIFF x x
ZREVRANGEBYSCORE x x
BZPOPMIN x x x
ZUNIONSTORE 0 x x
ZUNIONSTORE 1 x x
SPUBLISH
HINCRBY x x x
ZRANGEBYSCORE x x
DEL x x
HSCAN x x
SORT_RO x x
XPENDING x x
ZREMRANGEBYLEX x x
ZUNION x x
LREM x x
EXPIRE x x
LPOP x x x
EXISTS x
SREM x x
GETRANGE x x
XREAD x x
GEORADIUS 0 x x
GEORADIUS 1 x x
GEORADIUS 2 x x
ZRANGE x x
XGROUP SETID x x
MOVE x x x
BITPOS x x
HKEYS x x
GEORADIUS_RO x x
HSETNX x x
BITFIELD x x x
SETNX x x
BLMPOP x x x
COPY 0 x x
COPY 1 x x
LPUSHX x x
INCR x x x
XGROUP CREATECONSUMER x x
ZINTERSTORE 0 x x
ZINTERSTORE 1 x x
HINCRBYFLOAT x x x
LMPOP x x x
SUNIONSTORE 0 x x
SUNIONSTORE 1 x x
FCALL x x x
GEORADIUSBYMEMBER 0 x x
GEORADIUSBYMEMBER 1 x x
GEORADIUSBYMEMBER 2 x x
DUMP x x
HDEL x x
TTL x x
GEOHASH x x
RESTORE-ASKING x x
EVALSHA x x x
ZREVRANGEBYLEX x x
RESTORE x x
LPOS x x
ZREVRANGE x x
ZREM x x
OBJECT IDLETIME x
SETBIT x x x
SUNSUBSCRIBE
HGET x x
FCALL_RO x x
SUBSTR x x
MEMORY USAGE x
XDEL x x
PSETEX x x
BITCOUNT x x
LINSERT x x
MIGRATE 0 x x x
MIGRATE 1 x x x
ZREMRANGEBYSCORE x x
ZLEXCOUNT x x
SINTERCARD x
SDIFFSTORE 0 x x
SDIFFSTORE 1 x x
ZREVRANK x x
SMOVE 0 x x x
SMOVE 1 x x
ZCARD x
EXPIRETIME x x
RPUSH x x
MSET x x
LRANGE x x
ZPOPMAX x x x
XINFO STREAM x x
XAUTOCLAIM x x
GEOSEARCHSTORE 0 x x
GEOSEARCHSTORE 1 x x
XLEN x
ZRANDMEMBER x x
EVAL_RO x x
XGROUP CREATE x x
XINFO GROUPS x x
PFDEBUG x x
SISMEMBER x x
RPUSHX x x
BLPOP x x x
BITOP 0 x x
BITOP 1 x x
HGETALL x x
ZMPOP x x x
XCLAIM x x
PEXPIRETIME x x
XRANGE x x
GEORADIUSBYMEMBER_RO x x
TOUCH x
GET x x
GEOSEARCH x x
PFADD x x
ZSCORE x x
OBJECT ENCODING x
BITFIELD_RO x x
LINDEX x x
RPOP x x x
UNLINK x x
BZPOPMAX x x x
BRPOP x x x
SETEX x x
LPUSH x x
SETRANGE x x
INCRBY x x x
HLEN x
ZINCRBY x x x
OBJECT REFCOUNT x
SET x x x
PFCOUNT x x
BLMOVE 0 x x x
BLMOVE 1 x x
SINTERSTORE 0 x x
SINTERSTORE 1 x x
ZMSCORE x x
SCARD x
HEXISTS x
SMISMEMBER x x
SRANDMEMBER x x
EVAL x x x
ZRANK x x
ZPOPMIN x x x
STRLEN x
MGET x x
GEOPOS x x
GETDEL x x x
GETEX x x x
BRPOPLPUSH 0 x x x
BRPOPLPUSH 1 x x
PTTL x x
DECRBY x x x
WATCH
ZRANGEBYLEX x x
ZDIFFSTORE 0 x x
ZDIFFSTORE 1 x x
PEXPIRE x x
TYPE x
HRANDFIELD x x
INCRBYFLOAT x x x
GETSET x x x
LMOVE 0 x x x
LMOVE 1 x x

- Use INSERT for SADD, APPEND, HSETNX, SETNX (already did in SMOVE)
- Dont use ACCESS in PFCOUNT
- fix modules tests
- fix typo
@oranagra
Copy link
Member Author

update

Spec RO RW OW RM ACCESS UPDATE INSERT DELETE
MSETNX x x
HSETNX x x
SETNX x x
SADD x x
APPEND x x
PFCOUNT x

PFADD and SMOVE already used INSERT rather than UPDATE

@sundb
Copy link
Collaborator

sundb commented Jan 17, 2022

Should ZPOPMAX, ZPOPMIN and SPOP use DELETE?

@oranagra
Copy link
Member Author

yes.. all "POP"s should use delete. i did mark some that way, but not others.
i did a quick / sloppy job to start this process up, counting on incremental improvements.
the important part is to see that we're happy with the flags definition (which we should discuss in the other issue).
please let me know if you find other errors.

@oranagra oranagra linked an issue Jan 17, 2022 that may be closed by this pull request
@oranagra
Copy link
Member Author

update:

Spec RO RW OW RM ACCESS UPDATE INSERT DELETE
FCALL_RO x x

@oranagra oranagra changed the title new key-spec mess to sort New detailed key-spec flags (RO, RW, OW, RM, ACCESS, UPDATE, INSERT, DELETE) Jan 18, 2022
@oranagra oranagra added release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus labels Jan 18, 2022
@oranagra oranagra requested a review from yossigo January 18, 2022 13:03
@oranagra oranagra merged commit eef9c6b into redis:unstable Jan 18, 2022
@oranagra oranagra deleted the key-spec-mess branch January 18, 2022 14:00
enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Jan 19, 2022
In redis#10122, we modify the key spec flags to `RO` and `ACCESS`.
But forgot to call generate-command-code.py. Also formatted
it to follow the Python PEP8.
oranagra pushed a commit that referenced this pull request Jan 19, 2022
In #10122, we modify the key spec flags to `RO` and `ACCESS`.
But forgot to call generate-command-code.py. Also formatted
it to follow the Python PEP8.
@guybe7 guybe7 mentioned this pull request Jan 20, 2022
oranagra pushed a commit that referenced this pull request Jan 8, 2024
…command (#12917)

In #10122, we set the destination key's flag of SINTERSTORE to `RW`, 
however, this command doesn't actually read or modify the destination
key, just overwrites it.
Therefore, we change it to `OW` similarly to all other *STORE commands.
roggervalf pushed a commit to roggervalf/redis that referenced this pull request Feb 11, 2024
…command (redis#12917)

In redis#10122, we set the destination key's flag of SINTERSTORE to `RW`, 
however, this command doesn't actually read or modify the destination
key, just overwrites it.
Therefore, we change it to `OW` similarly to all other *STORE commands.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Sort out the mess with read keys on keyspecs
3 participants