Schema Engine: fix to account for Online DDL swapped tables #9324
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ine DDL Signed-off-by: Harish Mallipeddi <mvh@stripe.com>
… a second: for instance if a column is altered immediately after creation then we get test failures due to incorrect FIELD events which are based on the previous DDL. Unit tests: reload schema when tablets are added/deleted since the test schema engine is an external singleton Signed-off-by: Rohit Nayak <rohit@planetscale.com>
3 tasks
harshit-gangal
approved these changes
Dec 5, 2021
| if isInTablesMap && createTime < se.lastChange { | ||
| if isInTablesMap && createTime == tbl.CreateTime && !(createTime >= se.lastChange) { |
There was a problem hiding this comment.
createTime < se.lastChange looks same as !(createTime >= se.lastChange), can we keep the earlier check, looks easier to read.
harshit-gangal
requested review from
deepthi,
rafael,
shlomi-noach and
systay
as code owners
shlomi-noach
approved these changes
Dec 6, 2021
There was a problem hiding this comment.
Minor comment on my side; agree to @harshit-gangal 's comment.
Otherwise LGTM and the change looks tiny.
| @@ -343,7 +343,7 @@ func (se *Engine) reload(ctx context.Context) error { | |||
| // TODO(sougou); find a better way detect changed tables. This method | |||
| // seems unreliable. The endtoend test flags all tables as changed. | |||
There was a problem hiding this comment.
Let's also update this old comment by Sugu. Shall we clarify here as comment why the logic is as it is? Basically copy+paste the PR comment to explain about table creation and Online DDL etc.
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
rohit-nayak-ps
force-pushed
the
rn-schema-engine-reload-fix
branch
from
b4f96d3 to
a9f68d7
Compare
shlomi-noach
approved these changes
Dec 8, 2021
DeathBorn
pushed a commit
to vinted/vitess
that referenced
this pull request
Apr 13, 2023
Fix schema engine reload so it accounts for swapped tables during online DDL Signed-off-by: Harish Mallipeddi <mvh@stripe.com> Signed-off-by: Vilius Okockis <vilius.okockis@vinted.com>
DeathBorn
pushed a commit
to vinted/vitess
that referenced
this pull request
Apr 13, 2023
Reinstate lastChange logic to handle multiple DDLs that happen within a second: for instance if a column is altered immediately after creation then we get test failures due to incorrect FIELD events which are based on the previous DDL. Unit tests: reload schema when tablets are added/deleted since the test schema engine is an external singleton Signed-off-by: Rohit Nayak <rohit@planetscale.com> Signed-off-by: Vilius Okockis <vilius.okockis@vinted.com>
DeathBorn
pushed a commit
to vinted/vitess
that referenced
this pull request
Apr 13, 2023
Address review comments Signed-off-by: Rohit Nayak <rohit@planetscale.com> Signed-off-by: Vilius Okockis <vilius.okockis@vinted.com>
Merged
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This is based off of the external PR #9306 to try to fix the failing tests
This PR addresses the following bug in schema engine reload process:
information_schema.tables create_timefield to tell when a table is updated. Unlike the name might indicate, this field is updated when a structural DDL is performed on a table (e.g. a column add/delete).lastChange).create_timefor that table against this saved timestamp, and if it's older, we skip examining the table further for changes.create_timetimestamp for the table (it remains the original timestamp of when online DDL created the temporary table). If after this, but before the online DDL is done and the tables are swapped, a schema reload is run (in any of the various ways), we will record all the changes in schema at that point, and save the current time in the schema engine'slastChangefield.create_timefield is now older than the schema enginelastChangefield. We therefore do not update the schema engine's view of this table's schema.Changes:
lastChangetime in schema engine, we track thecreate_timeof every table explicitly. On a subsequent refresh, if we find thecreate_timeof the table reported by MySQL differs from what we have cached in-memory, then we will assume the table has changed and reload its schema.create_timebeing modified.Related Issue(s)
Fixes #9287
Checklist
Deployment Notes