Influxdb: syntax for CREATE DATABASE and USE should be congruent

[beckettsean]

Feature Request

Proposal: [Description of the feature]
Make the quoting behavior for USE <database> match the quoting rules for CREATE DATABASE

Current behavior: [What currently happens]

> create database 123
ERR: error parsing query: found 123, expected identifier at line 1, char 17
> create database "123"
> use 123
Using database 123
> use "123"
ERR: Database "123" doesn't exist. Run SHOW DATABASES for a list of existing databases.
> 

Desired behavior: [What you would like to happen]

use "123" should be valid regardless. That's valid InfluxQL for an identifier.

Ideally, use 123 should throw an error in that 123 isn't a valid identifier in InfluxQL. Strings starting with digits must be double-quoted if they are an identifier.

Use case: [Why is this important (helps with prioritizing requests)]

It's confusing and arbitrary, and probably easy to fix.

[jsternberg]

Part of the reason why this happens is because use is a client command rather than a server command, so it's not parsed by the same parser.

Unfortunately, there's no easy way to do that without including the InfluxQL parsing library inside the client and I don't think we want to do that. We can still try and pretend, but we won't be able to reuse the same code.

[gunnaraasen]

I think it would be nice to eventually use the InfluxQL parser in the CLI for error checking. It'll make the CLI much easier to use and allow invalid statements to be caught before they're sent to the database. Is there any reason we wouldn't want to directly reuse the InfluxQL code?

[jsternberg]

The main reason would be because we don't want the server to interpret those commands. It could be argued we don't want it to even acknowledge those commands exist, but we could allow it to parse client commands and the server would just ignore them.

The other reason at the moment is that the ast, parser, scanner, and query engine are all in the influxql package and that's a lot to pull in for just the CLI. We were planning to possibly break it up into different packages, but didn't have a compelling reason. I'll work a little bit on that today to see how feasible it is.

[jsternberg]

Also, a reason to not use the influxql library to parse the query would be because then when we add new syntax, an older client won't be able to parse the new query and send it to a newer server.

We might be able to reuse the scanner though and make a specific client parser.

[beckettsean]

Why can't we just alter the CLI to make both use "123" and use 123 valid commands? Why does it need to reject the first one as invalid? If someone actually has a database named "123" then they would have to run use "\"123\"". That would be congruent with the InfluxQL parser:

> create database \"123\"
ERR: error parsing query: found \, expected identifier at line 1, char 17
> create database "123"
> show databases
name: databases
---------------
name
telegraf
_internal
mydb
mytest
123

> create database ""123""
ERR: error parsing query: found 123, expected ; at line 1, char 19
> create database "\"123\""
> show databases
name: databases
---------------
name
telegraf
_internal
mydb
mytest
123
"123"

[beckettsean]

Not a huge deal, but this is the kind of inconsistency that will perpetually confuse users, I think.

As for adding the InfluxQL parser to the influx binary, I don't think size is a meaningful concern:

root@sean-stable:/usr/bin# ll -h influx*
-rwxr-xr-x 1 root root 6.6M Apr 20 17:00 influx*
-rwxr-xr-x 1 root root  17M Apr 20 17:00 influxd*
-rwxr-xr-x 1 root root  11M Apr 20 17:00 influx_inspect*
-rwxr-xr-x 1 root root 6.8M Apr 20 17:00 influx_stress*
-rwxr-xr-x 1 root root  13M Apr 20 17:00 influx_tsm*

We ship a 13MB binary that will be used at most twice by users, and will be irrelevant to anyone who isn't upgrading from an 0.9 instance. We ship another 17MB of binaries that will rarely be used by anyone. Adding a few MB to the CLI so it has the same parser doesn't seem onerous.

However, it is a significant concern that it would introduce versioning issues with the CLI. I'm not sure which issue is more pressing, the lack of consistency or preventing mis-matched versions from working together.

@jwilder @pauldix do you have a product opinion?

[gunnaraasen]

I don't see the problem with using the influxql package in the CLI. There wouldn't (and shouldn't) be any CLI stuff added to influxql itself. For instance the USE "db" command could use influxql's Scan() to check if there is a valid IDENT token.

Separating out the ast, parser, scanner, and query engine into a separate package would be nice but, as @beckettsean mentioned, I don't think binary bloat in the CLI will be a significant issue.

Reusing the parser to add nice syntax warnings would be a huge win for usability. We could easily add a flag or other check to disable warnings when interacting with InfluxDB versions that do not equal the CLI's version.

[jsternberg]

I think the bigger problem I have is with the versioning issues. We could try parsing the command and pass any commands that can't be parsed to the server rather than relying on the client, but this seems a bit ugly to me.

It might be what we have to do though.

[beckettsean]

I think it's more an accident than an intentional design that the CLI doesn't need to match versions with the server it queries. I don't know if it's a "feature" worth prioritizing over other issues.

What's the downside? If someone has different versions in different places, they would need to maintain two separate influx binaries on the machines that query both.

[jsternberg]

It's possible we could go that route. Postgres requires the CLI binary to match to my knowledge, but I always figured that was because of binary compatibility.

I did think it was a decent advantage that our CLI was so simple we didn't need to do that and I think that could be considered advantageous.

[pauldix]

Yeah, they should have matching versions of CLI and DB. At least, matching major versions once we get to 1.0. Also +1 on bringing InfluxQL into the CLI. I think there's more win there than downside.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically closed because it has not had recent activity. Please reopen if this issue is still important to you. Thank you for your contributions.