-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make DROP/CREATE DATABASE pluggable #7381
Conversation
Signed-off-by: Andres Taylor <[email protected]>
46c2f01
to
b7400d4
Compare
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! I just had one question about adding a wait step for DROP DATABASE
as well.
return &sqltypes.Result{RowsAffected: 1}, nil | ||
} | ||
|
||
err := plugin.DropDatabase(ctx, c.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had discussed that after the plugin says it's done, we should wait until vtgate observes that the keyspace is gone. The idea is to mimic the semantic in MySQL, which is that it's safe to re-create a database immediately after DROP DATABASE
returns. So we should probably check whatever signal we check when handling CREATE DATABASE
here and wait for that signal to say the keyspace doesn't exist. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had discussed that after the plugin says it's done, we should wait until vtgate observes that the keyspace is gone. The idea is to mimic the semantic in MySQL, which is that it's safe to re-create a database immediately after
DROP DATABASE
returns. So we should probably check whatever signal we check when handlingCREATE DATABASE
here and wait for that signal to say the keyspace doesn't exist. WDYT?
Just want to call it out that the current implementation depends on the plugin to take care of changing topo server metadata to tell us that keyspace is created or deleted.
I have added the necessary checks to ensure that.
Also you can change the query timeout with query like create database /*vt+ QUERY_TIMEOUT_MS=30000 */ <database_name>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to call it out that the current implementation depends on the plugin to take care of changing topo server metadata to tell us that keyspace is created or deleted.
That works for me. I just wanted vtgate to take care of the question, "Can this vtgate instance see the keyspace yet/still?" since the plugin can't determine that from topo alone.
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
return &sqltypes.Result{RowsAffected: 1}, nil | ||
} | ||
|
||
err := plugin.DropDatabase(ctx, c.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to call it out that the current implementation depends on the plugin to take care of changing topo server metadata to tell us that keyspace is created or deleted.
That works for me. I just wanted vtgate to take care of the question, "Can this vtgate instance see the keyspace yet/still?" since the plugin can't determine that from topo alone.
go/vt/vtgate/engine/dbddl.go
Outdated
if err != nil { | ||
return nil, err | ||
} | ||
for vcursor.FindKeyspace(c.name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like vcursor.FindKeyspace()
does a direct topo call, which checks the source of truth, but it doesn't check whether this particular vtgate instance has observed the change yet.
My goal was to guarantee that an immediate CREATE DATABASE x
after DROP DATABASE x
on the same vtgate session will succeed. It looks like CREATE DATABASE
checks vschema.KeyspaceExists(ksName)
to determine whether the keyspace exists. Wouldn't we want to check the same thing to guarantee that vtgate will allow it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to use vschema.
go/vt/vtgate/vcursor_impl.go
Outdated
|
||
func (vc *vcursorImpl) FindKeyspace(ks string) bool { | ||
_, err := vc.topoServer.GetKeyspace(vc.ctx, ks) | ||
return err == nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this mean that a transient error communicating with topo will make us assume the keyspace doesn't exist? It seems like anything that makes a remote call should return error
as well to be honest about the possibility that it might fail to get an answer at all. We would only translate the NotFound/NoNode error to false, nil
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now this is not an issue.
Signed-off-by: Harshit Gangal <[email protected]>
go/vt/vtgate/vcursor_impl.go
Outdated
} | ||
|
||
func (vc *vcursorImpl) FindKeyspace(ks string) bool { | ||
return vc.KeyspaceExists(ks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like KeyspaceExists()
reads from the vschema.Keyspaces
map without a mutex, which implies that the map is never updated by another goroutine. Doesn't that mean the result of this call will never change, even if vtgate has refreshed vschema in the background?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed that to see the executor view of VSchema.
The e2e test confirms that you can do create/drop multiple times in order.
RebuildVSchemaGraph
is necessary to update the executor's view.
… available Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Harshit Gangal <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
…ger available to be served Signed-off-by: Harshit Gangal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for doing the heavy lifting on waiting for vtgate to observe changes. This should make my job writing the plugin very easy! :)
Allows CREATE and DROP database to have behaviour controlled by a plugin.
Fixes #7525