Vinai/add drop pks #516
Vinai/add drop pks #516
Conversation
| // PrimaryKeyAlterableTable represents a table that supports primary key changes. | ||
| type PrimaryKeyAlterableTable interface { | ||
| Table | ||
| // CreatePrimaryKey creates a primary key for this table, using the provided parameters. | ||
| // Returns an error if the new primary key set is not compatible with the current table data | ||
| // or the table already has a key. | ||
| CreatePrimaryKey(ctx *Context, columns []string) error | ||
| // DropPrimaryKey drops a primary key on a table. Returns an error if that table does not have a key. | ||
| DropPrimaryKey(ctx *Context) error | ||
| } | ||
|
|
There was a problem hiding this comment.
This interface seems to collide somewhat with AlterableTable
There was a problem hiding this comment.
I think not in this case. AlterableTable only has ModifyColumn, which changes the definition of a single column.
If anything it kind of overlaps with IndexAlterableTable. Adding a primary key is a special case of adding a unique index (the first one for that table). This is actually exactly how MySQL handles that -- the first unique index added to a table without a primary key becomes the primary key.
I think I'm fine with this for now, if for no other reason than we don't support named primary keys, which you need to make this work with IndexAlterableTable
There was a problem hiding this comment.
Created a new interface on the side of caution. Not opinionated on either direction. Will keep this for now.
| // PrimaryKeyAlterableTable represents a table that supports primary key changes. | ||
| type PrimaryKeyAlterableTable interface { | ||
| Table | ||
| // CreatePrimaryKey creates a primary key for this table, using the provided parameters. | ||
| // Returns an error if the new primary key set is not compatible with the current table data | ||
| // or the table already has a key. | ||
| CreatePrimaryKey(ctx *Context, columns []string) error | ||
| // DropPrimaryKey drops a primary key on a table. Returns an error if that table does not have a key. | ||
| DropPrimaryKey(ctx *Context) error | ||
| } | ||
|
|
There was a problem hiding this comment.
I think not in this case. AlterableTable only has ModifyColumn, which changes the definition of a single column.
If anything it kind of overlaps with IndexAlterableTable. Adding a primary key is a special case of adding a unique index (the first one for that table). This is actually exactly how MySQL handles that -- the first unique index added to a table without a primary key becomes the primary key.
I think I'm fine with this for now, if for no other reason than we don't support named primary keys, which you need to make this work with IndexAlterableTable
sql/core.go
Outdated
| Table | ||
| // CreatePrimaryKey creates a primary key for this table, using the provided parameters. | ||
| // Returns an error if the new primary key set is not compatible with the current table data | ||
| // or the table already has a key. |
There was a problem hiding this comment.
Engine should check for the presence of a primary key
sql/core.go
Outdated
| // CreatePrimaryKey creates a primary key for this table, using the provided parameters. | ||
| // Returns an error if the new primary key set is not compatible with the current table data | ||
| // or the table already has a key. | ||
| CreatePrimaryKey(ctx *Context, columns []string) error |
There was a problem hiding this comment.
Copy the interface for IndexAlterableTable here, but with no indexName param:
CreateIndex(ctx *Context, indexName string, using IndexUsing, constraint IndexConstraint, columns []IndexColumn, comment string) error
There was a problem hiding this comment.
Does mysql even enforce names primary key constraints? When I was testing the given constraint name was never used in the information_schema.table_constraints name. Seems like this is just adding additional params that will never need to be used.
There was a problem hiding this comment.
After discussing it seems like IndexColumn is not enough. indexName, using, and index constraint are not needed
sql/errors.go
Outdated
|
|
||
| // ErrMultiplePrimaryKeyDefined is returned when a table invokes CreatePrimaryKey with a primary key already | ||
| // defined. | ||
| ErrMultiplePrimaryKeyDefined = errors.NewKind("error: Multiple primary key defined") |
sql/errors.go
Outdated
|
|
||
| // ErrWrongAutoKey is returned when a table invokes DropPrimaryKey without first removing the auto increment property | ||
| // (if it exists) on it. | ||
| ErrWrongAutoKey = errors.NewKind("error: incorrect table definition; there can be only one auto column and it must be defined as a key") |
enginetest/enginetests.go
Outdated
| }, nil, nil) | ||
|
|
||
| // Drop the original Pk, create an index, create a new primary key | ||
| TestQuery(t, harness, e, `ALTER TABLE t1 DROP PRIMARY KEY`, []sql.Row{}, nil, nil) |
There was a problem hiding this comment.
For queries that you aren't making any assertions on, you can just use RunQuery, not TestQuery
…er into vinai/add-drop-pks
| }, nil, nil) | ||
|
|
||
| // Assert that a duplicate row causes an alter table error | ||
| AssertErr(t, e, harness, `ALTER TABLE t1 ADD PRIMARY KEY (pk, v)`, sql.ErrPrimaryKeyViolation) |
There was a problem hiding this comment.
Should also check that the schema is unchanged after this
memory/table.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func insertIntoNewTable(t *Table, newSch sql.Schema) (*Table, error) { |
3 participants
No description provided.