PanicOnWarnings option to detect SQL warnings and fail the copy process#1
PanicOnWarnings option to detect SQL warnings and fail the copy process#1
Conversation
grodowski
force-pushed
the
grodowski/raise_on_warnings
branch
from
a4a4354 to
e78c3a7
Compare
grodowski
force-pushed
the
grodowski/raise_on_warnings
branch
2 times, most recently
from
d426b4c to
69d8158
Compare
go/logic/applier.go
Outdated
| continue | ||
| } | ||
| pkDuplicateSuffix := fmt.Sprintf("for key '%s.PRIMARY'", this.migrationContext.GetGhostTableName()) | ||
| if strings.HasPrefix(message, "Duplicate entry") && strings.HasSuffix(message, pkDuplicateSuffix) { |
There was a problem hiding this comment.
Possible to test this with localtests reliably?
go/sql/builder.go
Outdated
| %s.%s | ||
| where | ||
| %s and %s | ||
| with /* gh-ost %s.%s %s */ key_range as ( |
There was a problem hiding this comment.
CTEs not supported in mySQL 5 is the reason for the partly broken build. We could unwrap the CTE, I believe it would be equivalent perf-wise, but the duplication doesn't make me happy.
grodowski
changed the title
grodowski
force-pushed
the
grodowski/raise_on_warnings
branch
from
673576d to
067dbdf
Compare
localtests/panic-on-warnings-duplicate-unique-values-on-column-type-change/extra_args
Outdated
Show resolved
Hide resolved
| where | ||
| %s and %s | ||
| %s, | ||
| (select count(*) from ( |
There was a problem hiding this comment.
I'd be curious what kind of performance impact the changed query has. 🤔
I mainly asking because bad to slow people's migrations down if they don't intend to use the new flag.
There was a problem hiding this comment.
Yep, that's a good point. Intuitively, I think there shouldn't be much overhead on the count given how the old query looks. Do you think running explains on a reasonably-sized test table will be a good way to prove it?
There was a problem hiding this comment.
I was hoping they have some benchmarking setup, but doesn't look like it. Yeah, explain analyze on a big table would be a good place to start.
grodowski
force-pushed
the
grodowski/raise_on_warnings
branch
from
067dbdf to
7681d1c
Compare
- log insert warnings always - terminate if row count doesn't match and the PanicOnWarnings flag is set
…-type-change/extra_args Co-authored-by: Bastian Bartmann <accounts@bastianbartmann.de>
- documentation - remove dev.yml - remove unused variable
grodowski
force-pushed
the
grodowski/raise_on_warnings
branch
from
36eaf27 to
1487b5c
Compare
…s when renaming unique keys Error message formats are different across mysql distributions and versions
grodowski
force-pushed
the
grodowski/raise_on_warnings
branch
from
fe68db4 to
71a63c4
Compare
|
This is done: eedac87 and will be part of the next |
2 participants
A Pull Request should be associated with an Issue.
Related issue: https://github.com/github/gh-ost/issues/0123456789
Description
This PR [briefly explain what it does]
script/cibuildreturns with no formatting errors, build errors or unit test errors.