add RENAME DATABASE #4291

linearb · 2015-10-01T18:21:38Z

Support for RENAME DATABASE as in #4154

Comments welcome.

CQs not dealt with yet-- advice appreciated,
and not sure if correct proto version generated from meta.proto.

otoolep · 2015-10-01T19:00:02Z

Thanks @linearb -- I think there are more changes required to the TSDB layer. To see this you should add tests that query for data in a database, get results back, rename the database, and run the exact same query against the renamed database. You should get the same data back.

Make sense?

linearb · 2015-10-02T01:20:41Z

Thanks @otoolep . Adding that test is a good suggestion.
I'll have a harder look at the TSDB layer when I have a chance.

But are you sure TSDB changes are needed?--- it works when I do such a test by hand:

curl -G 'http://localhost:8086/query' --data-urlencode "q=CREATE DATABASE db_old"
{"results":[{}]}

curl -XPOST 'http://localhost:8086/write?db=db_old' -d 'cpu,host=server01,region=useast load=4 1434055565000000000'

curl -XPOST 'http://localhost:8086/write?db=db_old' -d 'cpu,host=server01,region=useast load=23 1434055569000000000'

curl -G http://localhost:8086/query?pretty=true --data-urlencode "db=db_old" --data-urlencode "q=SELECT * FROM cpu WHERE region='useast'"
{ "results": [ { "series": [ { "name": "cpu", "columns": [ "time", "host", "load", "region" ], "values": [ [ "2015-06-11T20:46:05Z", "server01", 4, "useast" ], [ "2015-06-11T20:46:09Z", "server01", 23, "useast" ] ] } ] } ] }

curl -G http://localhost:8086/query?pretty=true --data-urlencode "db=db_new" --data-urlencode "q=SELECT * FROM cpu WHERE region='us east'"
{"results":[{"error": "database not found: db_new"}]}

curl -G http://localhost:8086/query?pretty=true --data-urlencode "q=RENAME DATABASE db_old TO db_new"
{"results": [{}]}

curl -G http://localhost:8086/query?pretty=true --data-urlencode "db=db_new" --data-urlencode "q=SELECT * FROM cpu WHERE region='useast'"
{ "results": [ { "series": [ { "name": "cpu", "columns": [ "time", "host", "load", "region" ], "values": [ [ "2015-06-11T20:46:05Z", "server01", 4, "useast" ], [ "2015-06-11T20:46:09Z", "server01", 23, "useast" ] ] } ] } ] }

curl -G http://localhost:8086/query?pretty=true --data-urlencode "db=db_old" --data-urlencode "q=SELECT * FROM cpu WHERE region='useast'"
{ "results": [ { "error": "database not found: db_old" } ] }

otoolep · 2015-10-02T01:40:54Z

OK, I guess TSDB layer changes may not be required.

We still need that test however. Can you add it? We'll also need to run this command by the team.

linearb · 2015-10-02T01:47:52Z

Sure, I can add the test.

gunnaraasen · 2015-10-02T18:33:34Z

I'm not a fan of the RENAME DATABASE syntax. ALTER DATABASE feels more SQL-like. The full syntax would be something like:

ALTER DATABASE <old_db_name> RENAME TO <new_db_name>

Which roughly follows other SQL databases' renaming syntax and complements the existing ALTER RETENTION POLICY statement. Just my two cents.

beckettsean · 2015-10-02T19:57:57Z

+1 for ALTER DATABASE. That fits in with our other ALTER USER and ALTER RETENTION POLICY queries.

otoolep · 2015-10-02T20:02:51Z

Yeah, good call, it should be ALTER.

linearb · 2015-10-02T21:10:45Z

OK - ALTER it is.
Interestingly, mysql once had a RENAME DATABASE that was considered dangerous:
https://dev.mysql.com/doc/refman/5.1/en/rename-database.html

linearb · 2015-10-07T02:35:06Z

Realized that user privileges also needed updating on db rename.
So added something for that also.

Still unsure how to update CQs correctly, but if that's going to be
complicated perhaps better to split that off as a separate issue.

CHANGELOG.md updated
mergeable
Tests pass
Signed CLA

otoolep · 2015-10-07T18:05:28Z

cmd/influxd/run/server_test.go

@@ -67,17 +67,42 @@ func TestServer_DatabaseCommands(t *testing.T) {
 				exp:     `{"results":[{"series":[{"name":"databases","columns":["name"],"values":[["db0"],["db1"]]}]}]}`,
 			},
 			&Query{
+				name:    "rename database should succeed",


Should read alter database.

Actuall, I don't think it matters, now that I look at the tests.

otoolep · 2015-10-07T18:09:21Z

@linearb -- this looks really great, just some minor feedback. All community-contributions require 2 reviews by core developers, so we just need to wait for that too.

@dgnorton -- can you also review?

otoolep · 2015-10-07T18:10:22Z

meta/internal/meta.proto

@@ -105,6 +105,7 @@ message Command {
 		SetDataCommand                   = 17;
 		SetAdminPrivilegeCommand         = 18;
 		UpdateNodeCommand                = 19;
+                RenameDatabaseCommand            = 20;


Indentation looks slightly off here.

ok - will fix.

otoolep · 2015-10-07T18:11:43Z

Great test coverage, +1.

dgnorton · 2015-10-07T18:25:57Z

influxql/parser.go

+	// Check for required TO token.
+	if tok != TO {
+		return nil, newParseError(tokstr(tok, lit), []string{"TO"}, pos)
+	}


Parsing RENAME TO tokens can be condensed to...

// Parse required RENAME TO tokens. if err := p.parseTokens([]Token{RENAME, TO}); err != nil { return nil, err }

good point - will change.

linearb · 2015-10-09T17:46:35Z

@otoolep Suggested changes made.

otoolep · 2015-10-09T17:51:33Z

Thanks, I'll review now.

Can you rebase against master to remove the "merge" commits?

dgnorton · 2015-10-09T17:58:28Z

+1

otoolep · 2015-10-09T18:06:11Z

Thanks @linearb -- great work.

add RENAME DATABASE

dgnorton · 2015-10-20T13:41:51Z

We've decided to revert this change. See #4503 for details.

linearb · 2015-10-20T13:52:46Z

Too bad-- guess there was more involved here than any of us realized.

dgnorton · 2015-10-20T13:54:57Z

@linearb sorry we didn't catch this in the initial review. We really appreciate the effort you put in on it!

linearb · 2015-10-22T14:24:56Z

@dgnorton @otoolep Since I think this would be a good feature to get in, I'm willing to add
something to deal with problems raised in #4503 . Before going ahead, perhaps there
might be some consensus about which approach to take (or at least which to avoid)?

otoolep · 2015-10-22T16:56:46Z

Thanks @linearb -- we have not reached consensus on this issue. There are some difficult issues raised, with regards to renaming opening directories. The issue with some nodes crashing before the rename completes may also be difficult.

Perhaps some sort of simple aliasing is the best possible solution, which makes crash-recovery issue the simplest, and doesn't require any renaming of files on disk. The aliasing would be kept by Raft, and would exist in RAM only on all nodes. I believe this was suggested by @corylanou.

The mapping from alias to actual name would be referenced each time during queries and during writes. This would mean the change would touch much more of the code however.

otoolep · 2015-10-22T17:04:28Z

Actually, after some further discussions here, if instead the system used the database IDs as the filenames on disk, then it would be reasonably easy to support renames.

beckettsean · 2015-10-22T17:08:46Z

As long as there is a way for users to discover the mapping between database name and database ID that's a reasonable solution. I don't think it would be okay to obscure which database owned which files on disk.

otoolep · 2015-10-22T17:28:34Z

No reason why SHOW DATABASES wouldn't show this information, it would be easy enough.

beckettsean · 2015-10-22T17:45:56Z

That absolutely works for me. The only bad spot would be a case where the influxd process won't launch but we still want to know which database owns which shards. I can live with that in favor of supporting ALTER DATABASE, which will affect many more users than the corner case I described.

linearb · 2015-10-22T18:06:14Z

Sounds good. I'll put something together.

linearb · 2015-10-27T19:07:29Z

@otoolep Unless there is an internal database ID that I'm not seeing, I'll just add one like node id, or cluster id? Also, as changing the filenames is a breaking change, let me know if you have infrastructure for upgrades/downgrades to be aware of.

otoolep · 2015-10-27T19:10:46Z

No, it appears we don't have IDs for databases yet. You may need to add something to the DatabaseInfo type.

We also don't have any explicit code for upgrades and downgrades right now. We are open to suggestions, but I realise it can be a complex topic.

linearb mentioned this pull request Oct 2, 2015

[feature request] RENAME DATABASE #4154

Open

otoolep reviewed Oct 7, 2015
View reviewed changes

dgnorton reviewed Oct 7, 2015
View reviewed changes

linearb added 7 commits October 9, 2015 13:55

add RENAME DATABASE

7212bfc

add integration test for RENAME DATABASE

d2afd88

change syntax to ALTER DATABASE ... RENAME TO ...

668b5b9

update user privileges on database rename

1383d96

update changelog

218de0a

improvements from code review

37964a0

return error when database rename conflicts with continuous queries

60d2989

otoolep added a commit that referenced this pull request Oct 9, 2015

Merge pull request #4291 from linearb/linearb/rename-database

e00b822

add RENAME DATABASE

otoolep merged commit e00b822 into influxdata:master Oct 9, 2015

Inisfair referenced this pull request Oct 14, 2015

Update CHANGELOG.md

af217e6

otoolep mentioned this pull request Oct 19, 2015

Ensure DATABASE RENAME works across restarts #4503

Closed

gunnaraasen mentioned this pull request Mar 16, 2016

InfluxDB should restrict names/keys #6011

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add RENAME DATABASE #4291

add RENAME DATABASE #4291

linearb commented Oct 1, 2015

otoolep commented Oct 1, 2015

linearb commented Oct 2, 2015

otoolep commented Oct 2, 2015

linearb commented Oct 2, 2015

gunnaraasen commented Oct 2, 2015

beckettsean commented Oct 2, 2015

otoolep commented Oct 2, 2015

linearb commented Oct 2, 2015

linearb commented Oct 7, 2015

otoolep Oct 7, 2015

otoolep Oct 7, 2015

otoolep commented Oct 7, 2015

otoolep Oct 7, 2015

linearb Oct 7, 2015

otoolep commented Oct 7, 2015

dgnorton Oct 7, 2015

linearb Oct 7, 2015

linearb commented Oct 9, 2015

otoolep commented Oct 9, 2015

dgnorton commented Oct 9, 2015

otoolep commented Oct 9, 2015

dgnorton commented Oct 20, 2015

linearb commented Oct 20, 2015

dgnorton commented Oct 20, 2015

linearb commented Oct 22, 2015

otoolep commented Oct 22, 2015

otoolep commented Oct 22, 2015

beckettsean commented Oct 22, 2015

otoolep commented Oct 22, 2015

beckettsean commented Oct 22, 2015

linearb commented Oct 22, 2015

linearb commented Oct 27, 2015

otoolep commented Oct 27, 2015

add RENAME DATABASE #4291

add RENAME DATABASE #4291

Conversation

linearb commented Oct 1, 2015

otoolep commented Oct 1, 2015

linearb commented Oct 2, 2015

otoolep commented Oct 2, 2015

linearb commented Oct 2, 2015

gunnaraasen commented Oct 2, 2015

beckettsean commented Oct 2, 2015

otoolep commented Oct 2, 2015

linearb commented Oct 2, 2015

linearb commented Oct 7, 2015

otoolep Oct 7, 2015

Choose a reason for hiding this comment

otoolep Oct 7, 2015

Choose a reason for hiding this comment

otoolep commented Oct 7, 2015

otoolep Oct 7, 2015

Choose a reason for hiding this comment

linearb Oct 7, 2015

Choose a reason for hiding this comment

otoolep commented Oct 7, 2015

dgnorton Oct 7, 2015

Choose a reason for hiding this comment

linearb Oct 7, 2015

Choose a reason for hiding this comment

linearb commented Oct 9, 2015

otoolep commented Oct 9, 2015

dgnorton commented Oct 9, 2015

otoolep commented Oct 9, 2015

dgnorton commented Oct 20, 2015

linearb commented Oct 20, 2015

dgnorton commented Oct 20, 2015

linearb commented Oct 22, 2015

otoolep commented Oct 22, 2015

otoolep commented Oct 22, 2015

beckettsean commented Oct 22, 2015

otoolep commented Oct 22, 2015

beckettsean commented Oct 22, 2015

linearb commented Oct 22, 2015

linearb commented Oct 27, 2015

otoolep commented Oct 27, 2015