Skip to content

ApplySchema: allow DMLs#4389

Merged
sougou merged 1 commit intovitessio:masterfrom
planetscale:ss-schema-dml
Nov 27, 2018
Merged

ApplySchema: allow DMLs#4389
sougou merged 1 commit intovitessio:masterfrom
planetscale:ss-schema-dml

Conversation

@sougou
Copy link
Copy Markdown
Contributor

@sougou sougou commented Nov 25, 2018

ApplySchema was too restrictive. This is an interim change to
allow for DMLs if the keyspace is unsharded. Longer term,
we should allow statements to sharded keyspaces also, but
that requires either shard targeting or a way to forward
requests to vtgate.

Signed-off-by: Sugu Sougoumarane ssougou@gmail.com

ApplySchema was too restrictive. This is an interim change to
allow for DMLs if the keyspace is unsharded. Longer term,
we should allow statements to sharded keyspaces also, but
that requires either shard targeting or a way to forward
requests to vtgate.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Copy link
Copy Markdown
Member

@derekperkins derekperkins left a comment

Choose a reason for hiding this comment

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

LGTM. This will be really convenient for sequences.

@demmer
Copy link
Copy Markdown
Member

demmer commented Nov 26, 2018

I agree that we should remove the "schema change: '%s' does not introduce any table definition change" check, but I feel like hiding a DML or other query inside ApplySchema is somewhat subversive.

Since this boils down to calling ExecuteFetchAsDba on each of the masters, maybe it makes sense to add a -keyspace option to that command as an alternative to identifying the tablet alias?

In addition, I also don't understand why we need the restriction that this only works for unsharded keyspaces? Could we not just run the DML on each of the master tablets in turn? This would of course be dangerous for the operator if the DML changes the sharding key, but this is a backdoor anyway.

@sougou
Copy link
Copy Markdown
Contributor Author

sougou commented Nov 27, 2018

ApplySchema has a couple of convenient things that ExecuteFetch* functions don't have:

  • It parses and executes multiple statements.
  • It automatically identifies the master tablet, whereas ExecuteFetch is against a tablet alias. So, even -keyspace won't be sufficient.

I guess we can run updates and deletes on all tablets, and maybe prevent inserts?

The immediate use case for this is for initializing the sequence table. So, support for unsharded keyspace is sufficient. This will allow the minikube example I'm working on to be a single clean yaml file. We could even consider undoing this once we support a custom DDL for initializing sequences.

@derekperkins
Copy link
Copy Markdown
Member

Would this be helped by the topotools #4385 refactor? It seems like it is approaching a similar issue, wanting to execute commands at either vtctl or vtgate

Copy link
Copy Markdown
Member

@demmer demmer left a comment

Choose a reason for hiding this comment

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

I'm ok to go ahead with this as-is but I still feel like it's a misleading command name since it's not actually a schema change.

But this is just a naming thing and it's hardly the worst offender in the vtctl comand list, so I don't want to get hung up about it.

I also agree we should allow scatter UPDATE or DELETE in sharded keyspaces as well.

@sougou sougou merged commit ab75400 into vitessio:master Nov 27, 2018
@sougou sougou deleted the ss-schema-dml branch November 27, 2018 22:14
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.

3 participants