Testing and clarifying shared unique key requirement #358

Merged
merged 6 commits into from Jan 22, 2017

Projects

None yet

2 participants

@shlomi-noach
Collaborator

Storyline: #357

This PR adds the tests to confirm expected behavior of gh-ost, when requiring shared unique key between original/ghost tables.

To be followed up by a documentation update.

cc @ggunson

doc/requirements-and-limitations.md
@@ -31,7 +31,9 @@ The `SUPER` privilege is required for `STOP SLAVE`, `START SLAVE` operations. Th
- MySQL 5.7 `JSON` columns are not supported. They are likely to be supported shortly.
- The two _before_ & _after_ tables must share some `UNIQUE KEY`. Such key would be used by `gh-ost` to iterate the table.
- - As an example, if your table has a single `UNIQUE KEY` and no `PRIMARY KEY`, and you wish to replace it with a `PRIMARY KEY`, you will need two migrations: one to add the `PRIMARY KEY` (this migration will use the existing `UNIQUE KEY`), another to drop the now redundant `UNIQUE KEY` (this migration will use the `PRIMARY KEY`).
+ - What matters is not the key's _name_, but the columns covered by such key.
+ - As an example, if your table has a single `UNIQUE KEY my_unique_key(list, of, columns)` and no `PRIMARY KEY`, and you wish to replace the unique key with a `PRIMARY KEY`, you are good to go with a single migration: `--alter='DROP KEY my_unique_key, ADD PRIMARY KEY (list, of, columns)'`
@ggunson
ggunson Jan 10, 2017 Contributor

Depending on the outcome of testing, you should add what happens if you replace a PK (since the issue that sparked this got the PK demoted to unique and replaced with a different key). E.g. is it ok to switch out the PK if the original PK becomes a unique key? (Not necessarily obvious).

@shlomi-noach shlomi-noach requested a review from ggunson Jan 17, 2017
doc/requirements-and-limitations.md
@@ -30,8 +30,7 @@ The `SUPER` privilege is required for `STOP SLAVE`, `START SLAVE` operations. Th
- MySQL 5.7 `JSON` columns are not supported. They are likely to be supported shortly.
-- The two _before_ & _after_ tables must share some `UNIQUE KEY`. Such key would be used by `gh-ost` to iterate the table.
- - As an example, if your table has a single `UNIQUE KEY` and no `PRIMARY KEY`, and you wish to replace it with a `PRIMARY KEY`, you will need two migrations: one to add the `PRIMARY KEY` (this migration will use the existing `UNIQUE KEY`), another to drop the now redundant `UNIQUE KEY` (this migration will use the `PRIMARY KEY`).
+- The two _before_ & _after_ tables must share some `UNIQUE KEY`. Such key would be used by `gh-ost` to iterate the table. See [Read more](shared-key.md)
- The chosen migration key must not include columns with `NULL` values.
@ggunson
ggunson Jan 18, 2017 Contributor

I feel this is a sub-note of the previous note... It is about the migration key.

I might even want to say "the chosen migration key must include only not null columns". "columns with null values" could be more vague than referring to the column description being NOT NULL?

@shlomi-noach
shlomi-noach Jan 18, 2017 edited Collaborator

I might even want to say "the chosen migration key must include only not null columns".

However that is not the case. By default gh-ost does indeed only look for Unique Keys with not null columns, but supports null-able columns when the user takes responsibility and provides with --allow-nullable-unique-key.

For the migration to run correctly there must be no null values in the chosen key. gh-ost utilizes table definition when possible to know that is not the case, or trusts the user when this is not the case.

@ggunson
ggunson Jan 18, 2017 Contributor

Okay. I think this is really wordy, though, because it confused me. Since we're doing things in note form, maybe something like:

  • The migration key must not include columns with NULL values. This means either:

    1. The columns are NOT NULL, or
    2. The columns are nullable but don't contain any NULL values.
  • gh-ost will try to pick a migration key with non-nullable columns. It will not run if the only UNIQUE KEY includes nullable columns.

    • You may override this via --allow-nullable-unique-key but make sure there are no actual NULL values in those columns. Existing NULL values can't guarantee data integrity on the migrated table.
@shlomi-noach
shlomi-noach Jan 18, 2017 Collaborator

This is great wording and better than I can express myself. Would you mind pushing this into this branch?

Also, I don't know if you've seen the new file, shared-key.md? It really elaborates on the topic. I'm thinking we can just drop the entire paragraph from the requirements page altogether, given that it links to that page, and add your description in the shared-key.md file?

@ggunson
ggunson Jan 18, 2017 Contributor

Yeah, I saw shared-key.md too... The shared unique key and non-null values should be in the main page, I think. Can look at a moving around of text when it's not my bedtime.

@ggunson
ggunson Jan 19, 2017 Contributor

I made an edit but maybe it's still too wordy and I should have edited shared-key.md.

@shlomi-noach

Thanks -- OK to merge?

@ggunson
Contributor
ggunson commented Jan 20, 2017

Ok to merge for now.

@shlomi-noach shlomi-noach merged commit 31ba582 into master Jan 22, 2017

1 check passed

gh-ost-build-deploy-tarball Build #5222297 succeeded in 5s
Details
@shlomi-noach shlomi-noach deleted the tests-shared-unique-key branch Jan 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment