vtctl support for vschema ddls

[demmer]

Description

Add support for vschema DDL statements to be executed as vtctl commands.

Motivation

Making vschema changes through the DDL interface is safer and more granular than hand-editing the vschema json file. As such it is less likely that operators run into problems like #4288.

However, there are cases in which it is useful for these statements to be executed at vtctl / vtctld, so it would be beneficial to have the DDL syntax supported both at vtgate and as a vtctl command.

Details

The crux of the change adds new options to the ApplyVschema command to take a sql statement instead of the json specification, e.g.:

# vtctl ApplyVSchema -sql 'alter table test add vindex hash(id)' test_keyspace`

To implement this support, refactor the vschema ddl operations out of the vtgate executor and into a new topotools utility function to share it with vtctl.

Also add a -dry_run option to the ApplyVschema command to be able to see what the change will do to the vschema without applying it.

Finally, add a feature to set the Sharded: true flag on the vschema the first time a vindex is added to address a bootstrapping problem for new keyspaces. At some point we will want vschema ddl support for unsharded keyspaces, but that would necessarily involve a different syntax so we can cross that bridge later.

[rafael]

This looks good to me. Added couple comments.

[rafael]

I was thinking that it would make sense this to be driven by a flag. That way we can be explicit with the intent. If the flag is not provided and the Vindexes len is 0, adding a vindex should fail.

[demmer]

By definition, vindexes are only present in Sharded keyspaces, so I think the act of creating a vindex expresses the intent of whether we want

I have ideas about how we can extend the vschema ddl syntax to support unsharded keyspaces, but even then we won't have vindexes and I think above will hold.

[rafael]

I think a nice feature here is to delete the vindex altogether if no other tables are using it.

[demmer]

I think it's better to add an explicit syntax for dropping the vindex rather than implicit by dropping the last reference

[brirams]

this all seems reasonable to me but I don't know anything yet so take my review with a grain of salt.

provided some nits for consideration but no showstoppers

[brirams]

there seems to be a convention for a similarly named option(sql-file) in other spots. should we maintain that?

[demmer]

I was trying to be consistent with vschema_file but you're right we have this inconsistency unfortunately:

[SFO-M-MDEMMER03] -> vtctl --help | grep -- -file
  -db-credentials-file string
  -file_backup_storage_root string
  ApplySchema [-allow_long_unavailability] [-wait_slave_timeout=10s] {-sql=<sql> || -sql-file=<filename>} <keyspace>

[SFO-M-MDEMMER03] -> vtctl --help | grep -- _file
  -consul_auth_static_file string
  -grpc_auth_static_password_file string
  -mysql_auth_server_static_file string
  -pid_file string
  ApplyVSchema {-vschema=<vschema> || -vschema_file=<vschema file> || -sql=<sql> || -sql_file=<sql file>} [-cells=c1,c2,...] [-skip_rebuild] <keyspace>

[brirams]

same here(dry-run): https://github.com/tinyspeck/vitess/blob/b865bb068fe6f0c10d9825f8ba802e3869c5a86c/go/vt/vtctl/vtctl.go#L848

[demmer]

Ah good point. This one I will change.

[brirams]

do you need to add these options to the commands slice here?

[demmer]

Yes I do. Thanks for pointing this out.

[brirams]

could you squash these two lines:

vs = &vschemapb.Keyspace{}

[demmer]

thanks. fixed.

[brirams]

nit: rm : after parsing to match error format above.

[demmer]

fixed

[brirams]

TIOLI. big if/else blocks make my eyes cross:

	// apparently golang doesn't have an xor operator
	ddlMode := (*sql != "") != (*sqlFile != "")
	vschemaMode := (*vschema != "") != (*vschemaFile != "")

	switch {
	case ddlMode && vschemaMode:
		// both specified. return err
	case !ddlMode && !vschemaMode:
		// neither specified, return err
	}

	if ddlMode {

	}

	if vschemaMode {

	}

[demmer]

good suggestion. changed the names a bit but this is a much more readable cleanup

[rafael]

All comments have been addressed. This change looks good to me.