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

Allow adding sequence table to the vschema via ALTER VSCHEMA ADD SEQU… #5080

Conversation

kalfonso
Copy link
Contributor

Currently we can add a table to the vschema via "ALTER VSCHEMA", create a hash, lookup vindex, etc but there is no way to add a sequence table. A second PR will address referencing a sequence table from a sharded keyspace

Signed-off-by: Karel Alfonso Sague kalfonso@squareup.com

@kalfonso kalfonso requested a review from sougou as a code owner August 12, 2019 21:13
@@ -734,6 +734,9 @@ type DDL struct {
OptLike *OptLike
PartitionSpec *PartitionSpec

// IsVitessSequence is set for AddVschemaTableStr and refers to whether the table is a sequence table.
IsVitessSequence bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would just call it “IsSequence”. We know it’s Vitess. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you can have SQL sequences as well. For instance MariaDB allows "CREATE SEQUENCE" (https://mariadb.com/kb/en/library/create-sequence/). Initially wanted to differentiate.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. I think we don't need to support MariaDB sequences. Wdyt @sougou ?

Copy link
Member

Choose a reason for hiding this comment

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

IMO we don't need a special bit for this -- you can just re-use the VindexSpec and make the vindex "type" equal to "sequence.

@tirsen tirsen requested a review from demmer August 12, 2019 21:22
@tirsen
Copy link
Collaborator

tirsen commented Aug 12, 2019

FYI @demmer if you have an opinion on syntax

@sougou
Copy link
Contributor

sougou commented Aug 12, 2019

I'll take a look a this later, but here's an old document to use as reference: https://vitess.io/docs/reference/vitess-sequences/

@kalfonso
Copy link
Contributor Author

Thanks @sougou I'll update PR to reflect the sequence document instead of going via "alter vschema"

@tirsen
Copy link
Collaborator

tirsen commented Aug 13, 2019

I think we need an ALTER VSCHEMA statement for sequences.

For the future we could also add a CREATE SEQUENCE statements which collapses three things into the same statement:

  1. Create sequence table in the single shard (only works in an unsharded keyspace)
  2. Insert the initial row
  3. Runs the ALTER VSCHEMA statement in this PR

We won't ever be able to use such a statement though because the way we run DDL against our databases is completely different from how we insert data into them which is completely different from how we change the vschema. Because SOX.

So we still need an ALTER VSCHEMA statement for sequences.

@kalfonso kalfonso force-pushed the kalfonso.190810-alter-vschema-add-sequence-table branch 2 times, most recently from 3221b1e to fe4daac Compare August 13, 2019 07:21
@@ -1325,6 +1326,10 @@ alter_statement:
{
$$ = &DDL{Action: AddVschemaTableStr, Table: $5}
}
| ALTER VSCHEMA ADD SEQUENCE TABLE table_name
Copy link
Member

Choose a reason for hiding this comment

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

Syntactically since we eventually want to support CREATE SEQUENCE and not CREATE SEQUENCE TABLE, I think this should be ALTER VSCHEMA ADD SEQUENCE table_name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@demmer
Copy link
Member

demmer commented Aug 13, 2019

I think we need an ALTER VSCHEMA statement for sequences.
I agree

For the future we could also add a CREATE SEQUENCE statements which collapses three things into the same statement:
Yeah this would be nice too

…ENCE TABLE statement

Signed-off-by: Karel Alfonso Sague <kalfonso@squareup.com>
@kalfonso kalfonso force-pushed the kalfonso.190810-alter-vschema-add-sequence-table branch from fe4daac to 77089db Compare August 13, 2019 23:44
@sougou sougou merged commit 08d588e into vitessio:master Aug 14, 2019
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.

None yet

4 participants