-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Online DDL: ANALYZE
the shadow table before cut-over
#17201
Online DDL: ANALYZE
the shadow table before cut-over
#17201
Conversation
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
go/vt/vttablet/onlineddl/executor.go
Outdated
@@ -967,13 +999,13 @@ func (e *Executor) cutOverVReplMigration(ctx context.Context, s *VReplStream, sh | |||
if err != nil { | |||
return vterrors.Wrapf(err, "failed getting locking connection") | |||
} | |||
defer lockConn.Recycle() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an unrelated change that I spotted, and I will also make a separate PR just for this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reference: #17207
This is a great idea. We've sometimes had to run this manually after cut-over to correct query plans. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17201 +/- ##
==========================================
- Coverage 67.34% 67.33% -0.01%
==========================================
Files 1570 1570
Lines 252734 252763 +29
==========================================
- Hits 170204 170200 -4
- Misses 82530 82563 +33 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Had the one Wrapf nit and then the larger question about the naming, since I think shadow
would be better than vrepl
since I didn't see anything vreplication specific. No?
@@ -72,6 +72,7 @@ CREATE TABLE IF NOT EXISTS schema_migrations | |||
`is_immediate_operation` tinyint unsigned NOT NULL DEFAULT '0', | |||
`reviewed_timestamp` timestamp NULL DEFAULT NULL, | |||
`ready_to_complete_timestamp` timestamp NULL DEFAULT NULL, | |||
`vrepl_analyzed_timestamp` timestamp NULL DEFAULT NULL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the naming, this isn't shadow_analyzed_timestamp
? I didn't think that this was specific to the vreplication target table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to shadow_analyzed_timestamp
.
go/vt/vttablet/onlineddl/executor.go
Outdated
preparation := func() error { | ||
preparationsConn, err := e.pool.Get(ctx, nil) | ||
if err != nil { | ||
return vterrors.Wrapf(err, "failed getting preparation connection") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should just be Wrap. The linters are starting to get more strict about this. Same thing for the other instances of Wrapf w/o any format options/params. I know it's nitty, but otherwise people with newer linters locally will encounter the warnings/errors and have to fix them (e.g. simply when merging in the latest from main
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed (in two places).
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
…ale/vitess into onlineddl-cutover-analyze-table Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Signed-off-by: Renan Rangel <rrangel@slack-corp.com>
Description
See story in #17200
Online DDL will now run
ANALYZE TABLE
on the shadow table before cut-over. Note:ANALYZE
once, on the first cut-over. Successive cut-overs (in case of continued timeouts) will not runANALYZE TABLE
.lock_wait_timeout
to prevent excessive locking.ANALYZE
is normally fast, but if runs long, it could cause vreplication lag. We therefore run this before locking the tables, and just before creating the sentry table, after which we issuewaitForPos
.ANALYZE
is best effort. If it fails (timeout, likely) we take note but proceed with cut-over.Related Issue(s)
Fixes #17200
Checklist
Deployment Notes