Skip to content
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

vindex ddl statements #3498

Merged
merged 12 commits into from
Mar 19, 2018
Merged

vindex ddl statements #3498

merged 12 commits into from
Mar 19, 2018

Conversation

demmer
Copy link
Member

@demmer demmer commented Dec 22, 2017

Overview

Implement support for DDL statements to modify the vschema as described in #3459.

Notes

Syntax

This adds support for all the following statements:

  • CREATE VINDEX <name> USING <type> [WITH params]
  • DROP VINDEX <name>
  • ALTER TABLE <table> ADD VINDEX <name>
  • ALTER TABLE <table> ADD VINDEX <name> USING <type> [WITH params]
  • ALTER TABLE <table> DROP VINDEX <name>

I decided not to implement DROP VINDEX as a standalone statement since it would require verifying that the vindex isn't in use by any tables. If this becomes important we can always do this in a separate PR.

Topology Changes

In the original PR we proposed having vtgate call out to vtctld to make the requested topo changes. However since there is currently no linkage between vtgate and a vtctld instance, I instead made changes in the srvtopo interfaces so the vtgate can make the requested changes directly.

This assumes that any environment where this feature is used allows vtgate to talk to all topo servers (including those in other cells).

I also created a new srvtopo.VSchemaServer interface, implemented that within ResilientServer and pass both that and the original srvtopo.Server interface down into the vtgate.

Access Control

To restrict which users can make vindex changes using DDLs, I added a very simple access control mechanism. A new flag -vschema_ddl_authorized_users contains a comma separated list of usernames or "%" to represent all users, and the caller ID is compared to this list of usernames when executing the DDL statements.

@alainjobart
Copy link
Contributor

This is really cool. At some point, it will be nice to have all DB schemas (regular MySQL schemas and vindexes) consistently created by just running SQL statements. Then we can control everything with our regular schema management tools...

@demmer demmer force-pushed the vindex-ddl-statements branch 2 times, most recently from f8f639b to dc0101f Compare January 16, 2018 19:37
@demmer demmer force-pushed the vindex-ddl-statements branch 7 times, most recently from 55cbb36 to c15033b Compare February 28, 2018 00:05
@demmer
Copy link
Member Author

demmer commented Feb 28, 2018

@alainjobart / @sougou I spent some time over the last couple days dusting off and polishing up this PR.

I updated the descriptions here and in #3459 to reflect the new syntax and implementation.

@alainjobart
Copy link
Contributor

On the srvtopo.Server split, @demmer and I talked for a bit about it, and we agreed it was a bit confusing. srvtopo.Server is supposed to be a read-only access to the local topo, and topo.Server is the place to have read-write methods. The new vschema manager was using bits and pieces of each, and it is confusing.

So we agreed to, instead:

  • add a method GetTopoServer() *topo.Server to the srvtopo.Server interface. That way it's easy from a srvtopo.Server to gets its underlying topo.Server.
  • in all places where we pass both, now just pass srvtopo.Server.
  • call the read-write methods, when needed, on the topo.Server.

@demmer demmer force-pushed the vindex-ddl-statements branch 3 times, most recently from 780aedb to 22621b5 Compare March 19, 2018 13:34
@demmer
Copy link
Member Author

demmer commented Mar 19, 2018

@alainjobart / @sougou I finally got a bit of time to revive this PR.

I've rebased against upstream and adapted to the change that we agreed on in #3740. This should be ready for final review and then we can get it in!

The biggest implication of this is that I ended up needing to change sandboxTopo so that it implements watches using a real memorytopo server. To minimize the impact of this, I made this optional since it does depend on consistency in the set of cells used in the tests. See 21df434 for the detailed changes.

Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
This seemed cleaner than defining a new keyword

Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
Pull the vschema watching logic out of the executor and into a new
vschema manager which manages the watch for SrvVSchema updates and
support for updating the topo with the changed vschema.

Also update the sandboxTopo with optional support to use a backing
memorytopo.Server to enable watches to be properly simulated.

Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
In addition to subscribing to the simulated watch on the SrvVschema,
add a polling loop to the test to wait for the change to also be
propagated to the VindexManager's watch.

Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
This adds support for constructs like:
`alter table t add vindex v (c1,c2) using type on owner with p1=p2`

Along the way change the grammar for create vindex so that the vindex
type is preceded with `USING` and the owner is preceded with `ON`.

Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
This required adding a bunch more keywords to the grammar to handle
all the various options for `alter table add`.

Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
Instead of using the opaque `on <table>` syntax, add a special case
"owner=table" parameter.

Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
To ensure that only allowed users can execute vindex DDLs, add a
new flag -vschema_ddl_authorized_users which contains a comma
separated list of usernames or '%' to represent all users, along
with authorization hooks to make sure only allowed users can
execute DDL statements that modify the vschema.

Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
Also refactor a bunch of the implementation and beef up the testing

Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
Replace the goroutine implementation with a more straightforward
test and don't use `column` as an unquoted field name since it is
no longer supported by the grammar.

Signed-off-by: Michael Demmer <mdemmer@slack-corp.com>
Copy link
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Good structure.

@sougou sougou merged commit 71997da into vitessio:master Mar 19, 2018
frouioui pushed a commit to planetscale/vitess that referenced this pull request Mar 26, 2024
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.

4 participants