Skip to content

add a command which can return the dbnumber that the client selected#8000

Closed
631086083 wants to merge 2 commits intoredis:6.0from
631086083:6.0
Closed

add a command which can return the dbnumber that the client selected#8000
631086083 wants to merge 2 commits intoredis:6.0from
631086083:6.0

Conversation

@631086083
Copy link

Add a command to redis, which can return the database currently used by the redis client, and call it through dbpos

@oranagra
Copy link
Member

@631086083 thanks for the suggestion.
@itamarhaber I wonder why this didn't come up in the past? Or maybe it did?

A few initial comments:

  1. PRs should me made against the unstable branch not the 6.0 release branch.
  2. The command needs an ok-loading flag too
  3. The change in processCommand is undesired.
  4. The command doesn't need to check the argc, instead the arity field in the command table should be set.
  5. I don't like the name of the command ("pos"), maybe SELECTEDDB? is better

@tporadowski
Copy link
Contributor

Current documentation for SELECT says at the end:

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.

@oranagra: If this is really required (clients indeed must track this themselves, in worst case can be extracted from CLIENT LIST) - then I would say it more belongs to CLIENT set of commands, e.g. CLIENT DBNUM or similar, rather than its own command.

@soloestoy
Copy link
Contributor

I would say it more belongs to CLIENT set of commands, e.g. CLIENT DBNUM or similar, rather than its own command.

I agree with @tporadowski , CLIENT DBNUM is similar with CLIENT ID.

BTW, CLIENT LIST reply contains all these informations, but it's not easy to get the current client itself, I think here we need a new subcommand in CLIENT, maybe called CLIENT SELFINFO, to list current client only, what do you think @oranagra ?

@oranagra
Copy link
Member

CLIENT LIST is an admin introspection command, so it's OK that it shows things about the internals that are not normally the business of the application.

I do agree that we want to add a CLIENT INFO command that will show the same info CLIENT list shows, on a single client. current client by default, and maybe others if a specific ID(s) is provided.

But I don't think as want to add a specific CLIENT subcommand to get the selected db.
If the application need is, it should be a separate command like in this PR (with a slightly different name).
But the documentation says the client must track it, and we did go for many years without such a command, so I wonder if/why we wanna add such a thing, and if it was discussed in the past.

@hwware
Copy link
Contributor

hwware commented Oct 31, 2020

hi @oranagra @soloestoy for the client info one, how about instead of providing a separate command, we extend the CLIENT LIST command for multiple arguments? which is like CLIENT LIST [myself / id ].. user can specify the id or myself for the specific client information which they want. just not sure whether that brings extra compleity. for CLIENT DBNUM, not sure we need this or not, since everytime before we do something we do a select db first in order to avoid any surprise. or am i missing something?

@soloestoy
Copy link
Contributor

soloestoy commented Oct 31, 2020

@hwware at first I wanna that too, but maybe it's a little complex, since now CLIENT LIST already has type option.

@hwware
Copy link
Contributor

hwware commented Oct 31, 2020

@soloestoy I think the type option hit the argc == 4 case, but if we can provide CLIENT LIST [myself / id ] that should be a argc == 3 case right? If we return multiple ids that will definitely hit the complexity, but for a single argument to specify id/myself, IMHO the implementation should be trivial and shouldn't break the backward compatability.. how do you think? But maybe we need to refresh the documentation a bit if we do that.

@itamarhaber
Copy link
Member

@oranagra I wonder why this didn't come up in the past? Or maybe it did?

As @tporadowski said, this should be tracked by the client. I'm not sure what would this be used for other than in development and from redis-cli. @631086083 - can you share the motivation for adding this?

Anyway, if needed, this could belong in the CLIENT command as discussed above, but my problem with that is that this command is so overloaded that it is a real pain, especially given ACL. Another thought is to use SELECT for this purpose, it being the main db API. One way would be to have it return the current db id when called without any arguments (currently an arity error). Alternatively, something like SELECT * could do the trick.

@631086083
Copy link
Author

@631086083 thanks for the suggestion.
@itamarhaber I wonder why this didn't come up in the past? Or maybe it did?

A few initial comments:

  1. PRs should me made against the unstable branch not the 6.0 release branch.
  2. The command needs an ok-loading flag too
  3. The change in processCommand is undesired.
  4. The command doesn't need to check the argc, instead the arity field in the command table should be set.
  5. I don't like the name of the command ("pos"), maybe SELECTEDDB? is better

I'm very sorry, this is the second time I submit PR on GitHub. I'm not very proficient. Thank you very much for your advice. I will do better next time.
At present, there is no command that can return the database selected by our client, which requires the user or the language used to record; therefore, we should be very careful in the interactive programming and program development, and we need to use selectdb command to confirmwhen doing some special operations. If you forget which databases are currently used, you need to go through it again and find some keys to determine the current databases

@631086083
Copy link
Author

@oranagra I wonder why this didn't come up in the past? Or maybe it did?

As @tporadowski said, this should be tracked by the client. I'm not sure what would this be used for other than in development and from redis-cli. @631086083 - can you share the motivation for adding this?

Anyway, if needed, this could belong in the CLIENT command as discussed above, but my problem with that is that this command is so overloaded that it is a real pain, especially given ACL. Another thought is to use SELECT for this purpose, it being the main db API. One way would be to have it return the current db id when called without any arguments (currently an arity error). Alternatively, something like SELECT * could do the trick.

At present, there is no command that can return the database selected by our client, which requires the user or the language used to record; therefore, we should be very careful in the interactive programming and program development, and we need to use selectdb command to confirm when doing some special operations. If you forget which databases are currently used, you need to go through it again and find some keys to determine the current databases,Moreover, the implementation of this command is relatively simple, and can return the required information in O (1) time complexity

@oranagra
Copy link
Member

oranagra commented Nov 1, 2020

@631086083 doing SELECT command to confirm the desired db is selected is better, and faster than running a command to query that (SELECT can be used in a pipeline, no need to wait for a round trip and wait for the result).
Did you mean such a command is needed for an interactive user / console (like redis-cli) or a program using a client library? if you're using a client library, it can track it and won't forget.

unless maybe some script changed the selected DB, or maybe it is needed before using a command such as MOVE that takes a DB number as argument.

@itamarhaber I like your idea of SELECT taking a special argument to return the selected DB instead, but it's not ideal that it changes the response type. but since the response type is explicitly controlled by the user input, i guess we can do that. can you come up with an example of another command that does such a thing?

@hwware i don't like to mess with the already complicated CLIENT LIST. i think the best approach is add CLIENT INFO, with or without an optional list of input IDs.

@itamarhaber
Copy link
Member

@631086083 thanks for confirming my suspicions - this is a developer's tool and not a "real" command. Like KEYS for example. I can live with that.

can you come up with an example of another command that does such a thing?

@oranagra Basically all(most?) commands with subcommands are like that, but the most quirky is COMMAND which accepts no arguments or a subcommand. Following the same logic we can say SELECT [dbnum] and the reply's type will change accordingly instead of erroring when no argument is provided. Hackish, yes, but we don't have a consistent API definition so anything goes :)

That said, with RESP3, clients aren't expected to know the reply's type beforehand, so that's kind of ok.

@oranagra
Copy link
Member

oranagra commented Nov 1, 2020

@itamarhaber i don't consider sub-commands to be an example for that (a command that returns different types based on arguments). i consider each sub-command to be a command of it's own.
I think we have such examples in other commands (just can't currently come up with an example), but consider this comment as an example:
#766 (comment)

so to sum up my opinion:
i vote for:

  1. if SELECT gets a "?" as the db index argument it returns an integer response. p.s. another concern would have been if SELECT was flagged as a write and we now turn it into a read command, but in fact it is neither.
  2. add CLIENT INFO that returns the current client INFO, if we want we can let it take an optional list of client IDs in which case it returns an array (we probably want to make it return an array even if it gets no arguments)

@itamarhaber
Copy link
Member

I think we have such examples in other commands (just can't currently come up with an example)

So SET and SORT are quirky like that and reply type changes as a result of switches/arguments.

I agree with 1. WRT 2 it would be an ACL nightmare to restrict a client to call only its info and not others, so maybe two subcommands (e.g. variadic INFO and MYINFO).

@631086083 631086083 closed this Nov 2, 2020
@631086083
Copy link
Author

Thank you very much, I have learned a lot here, and I have a deeper understanding of the redis source code, especially the new mechanism of ACL. Next, I will read the redis source code in depth (^▽^ )

@oranagra
Copy link
Member

oranagra commented Nov 2, 2020

@631086083 do you want to suggest PRs for the two commands (together with tests and documentation)? or should someone else pick it up?

@hpatro
Copy link
Contributor

hpatro commented Dec 7, 2020

@oranagra I see a tiny problem with modifying the SELECT command behaviour. For SELECT ? to return the current database. The customer has to add this sub command to the ACL entry if the customer is already using something like +@all -SELECT +SELECT|0 to +@all -SELECT +SELECT|? +SELECT|0. However with a separate command I feel it's cleaner.

CLIENT INFO definitely makes a lot of sense.

@oranagra
Copy link
Member

oranagra commented Dec 7, 2020

i think the ACL attempt to block SELECT|1 (as if 1 is a sub-command) is somewhat of an abuse, but considering that we don't have any other solution for this use case, i suppose a separate command isn't that bad.

@631086083
Copy link
Author

@631086083 do you want to suggest PRs for the two commands (together with tests and documentation)? or should someone else pick it up?

I’m very sorry, I’m not very familiar with github, I closed the issue by mistake and missed this message.It looks like this order has been completed, thank you for your trust

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

Successfully merging this pull request may close these issues.

7 participants