Skip to content

Add currdb command to get current db value.#8139

Closed
hpatro wants to merge 1 commit intoredis:unstablefrom
hpatro:currdb
Closed

Add currdb command to get current db value.#8139
hpatro wants to merge 1 commit intoredis:unstablefrom
hpatro:currdb

Conversation

@hpatro
Copy link
Contributor

@hpatro hpatro commented Dec 6, 2020

Currently during cli connect of Redis CLI, if it fails to SELECT a database (due to ACL permissions), the user lands onto database 0. However the Redis CLI prompt states otherwise which is confusing to the end user. For e.g.

$ ./redis-cli
127.0.0.1:6379> acl setuser mydefault on nopass +@all -select +select|5
OK
127.0.0.1:6379> acl list
1) "user default on nopass ~* &* +@all"
2) "user mydefault on nopass &* +@all -select +select|5"
127.0.0.1:6379>
$ ./redis-cli --user mydefault --pass a -n 1
Warning: Using a password with '-a' or '-u' option on the command line interface may not be safe.
127.0.0.1:6379[1]> client list
id=8 addr=127.0.0.1:61906 laddr=127.0.0.1:6379 fd=8 name= age=10 idle=0 flags=N db=0 sub=0 psub=0 multi=-1 qbuf=26 qbuf-free=45024 argv-mem=10 obl=0 oll=0 omem=0 tot-mem=62490 events=r cmd=client user=mydefault redir=-1
127.0.0.1:6379[1]>

As we can see in the above scenario, the current client is pointing to 0 db index instead of 1.

After the proposed changes, redis-cli warns about the failure of selection of database as well as displays the current database index the client is pointing to.

$ ./redis-cli
127.0.0.1:6379> acl setuser mydefault on nopass +@all -select +select|5
OK
127.0.0.1:6379> acl list
1) "user default on nopass ~* &* +@all"
2) "user mydefault on nopass &* +@all -select +select|5"
$ ./redis-cli --user mydefault --pass a -n 1
Warning: Using a password with '-a' or '-u' option on the command line interface may not be safe.
Warning: SELECT failed
127.0.0.1:6379> client list
id=10 addr=127.0.0.1:62661 laddr=127.0.0.1:6379 fd=8 name= age=5 idle=0 flags=N db=0 sub=0 psub=0 multi=-1 qbuf=26 qbuf-free=45024 argv-mem=10 obl=0 oll=0 omem=0 tot-mem=62490 events=r cmd=client user=mydefault redir=-1
127.0.0.1:6379> CURRDB
(integer) 0
127.0.0.1:6379> SELECT 1
(error) NOPERM this user has no permissions to run the 'select' command or its subcommand
127.0.0.1:6379> SELECT 5
OK
127.0.0.1:6379[5]> CURRDB
(integer) 5

@madolson
Copy link
Contributor

madolson commented Dec 6, 2020

We can also get this from client info, there is a new field coming in #8113, so it might not be worth adding a dedicated command for it.

We also need to make sure this works with older versions of Redis as well, since you can run the newer CLI with the older redis version that doesn't have the command.

@oranagra
Copy link
Member

oranagra commented Dec 6, 2020

note that this was discussed in #8000 (we concluded we wanna add CLIENT INFO, and considered to add an option to do SELECT ?)

Also regarding the example given (ACL fails the SELECT that is issued by redis-cli), it's in some way related to #8136 and #8099

@hpatro
Copy link
Contributor Author

hpatro commented Dec 7, 2020

Thanks @madolson and @oranagra for sharing the context. I didn't see the thread earlier. Let me continue the discussion in #8000.

@itamarhaber
Copy link
Member

itamarhaber commented Dec 8, 2020

Also note this from the SELECT docs:

Since the currently selected database is a property of the connection, clients should track the currently selected database and re-select it on reconnection. While there is no command in order to query the selected database in the current connection, the CLIENT LIST output shows, for each client, the currently selected database.

@hwware
Copy link
Contributor

hwware commented Dec 9, 2020 •
edited
Loading

I remembered long time ago I opened an issue similar to this: #6727. I think currently cli implementation caching the db number in client side, but in some cases, from the reply we cannot guarantee the select command was executed successfully, therefore causing inconsistencies. If there is some way we can get current db info and refactor cli code, we can elimiate this issue.

@hwware hwware mentioned this pull request Dec 9, 2020
Merged
@madolson
Copy link
Contributor

madolson commented Dec 22, 2020

@hpatro Can we drop this PR?

@hpatro hpatro closed this Dec 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@hpatro @madolson @oranagra @itamarhaber @hwware