Skip to content
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.

Sign up for GitHub

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

Cleanup whitespace and comments in SQL query text #1246

Merged
merged 8 commits into from
Dec 8, 2023
Merged

Cleanup whitespace and comments in SQL query text #1246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1e1e42d
Cleanup whitespace in SQL query text
timvaillancourt Jan 11, 2023
dae0482
cleanup
timvaillancourt Jan 11, 2023
4042892
Add indent
timvaillancourt Jan 11, 2023
6f1de5d
Update unit tests
timvaillancourt Jan 11, 2023
704c412
Update unit tests, pt 2
timvaillancourt Jan 11, 2023
7766a35
Fix tweaks
timvaillancourt Jan 11, 2023
eac4234
Merge branch 'master' into fix-sql-whitespace
timvaillancourt Dec 8, 2023
17e8fba
Fix merge conflict resolution
timvaillancourt Dec 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 30 additions & 29 deletions go/logic/applier.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ func (this *Applier) InitDBConnections() (err error) {

// validateAndReadTimeZone potentially reads server time-zone
func (this *Applier) validateAndReadTimeZone() error {
query := `select @@global.time_zone`
query := `select /* gh-ost */ @@global.time_zone`
if err := this.db.QueryRow(query).Scan(&this.migrationContext.ApplierTimeZone); err != nil {
return err
}
Expand Down Expand Up @@ -392,14 +392,15 @@ func (this *Applier) WriteChangelog(hint, value string) (string, error) {
explicitId = 3
}
query := fmt.Sprintf(`
insert /* gh-ost */ into %s.%s
(id, hint, value)
values
(NULLIF(?, 0), ?, ?)
on duplicate key update
last_update=NOW(),
value=VALUES(value)
`,
insert /* gh-ost */
into
%s.%s
(id, hint, value)
values
(NULLIF(?, 0), ?, ?)
on duplicate key update
last_update=NOW(),
value=VALUES(value)`,
sql.EscapeName(this.migrationContext.DatabaseName),
sql.EscapeName(this.migrationContext.GetChangelogTableName()),
)
Expand Down Expand Up @@ -854,7 +855,7 @@ func (this *Applier) GetSessionLockName(sessionId int64) string {
// ExpectUsedLock expects the special hint voluntary lock to exist on given session
func (this *Applier) ExpectUsedLock(sessionId int64) error {
var result int64
query := `select is_used_lock(?)`
query := `select /* gh-ost */ is_used_lock(?)`
lockName := this.GetSessionLockName(sessionId)
this.migrationContext.Log.Infof("Checking session lock: %s", lockName)
if err := this.db.QueryRow(query, lockName).Scan(&result); err != nil || result != sessionId {
Expand All @@ -867,14 +868,14 @@ func (this *Applier) ExpectUsedLock(sessionId int64) error {
func (this *Applier) ExpectProcess(sessionId int64, stateHint, infoHint string) error {
found := false
query := `
select id
from information_schema.processlist
where
id != connection_id()
and ? in (0, id)
and state like concat('%', ?, '%')
and info like concat('%', ?, '%')
`
select /* gh-ost */ id
from
information_schema.processlist
where
id != connection_id()
and ? in (0, id)
and state like concat('%', ?, '%')
and info like concat('%', ?, '%')`
err := sqlutils.QueryRowsMap(this.db, query, func(m sqlutils.RowMap) error {
found = true
return nil
Expand Down Expand Up @@ -912,10 +913,10 @@ func (this *Applier) CreateAtomicCutOverSentryTable() error {
}
tableName := this.migrationContext.GetOldTableName()

query := fmt.Sprintf(`create /* gh-ost */ table %s.%s (
query := fmt.Sprintf(`
create /* gh-ost */ table %s.%s (
id int auto_increment primary key
) engine=%s comment='%s'
`,
) engine=%s comment='%s'`,
sql.EscapeName(this.migrationContext.DatabaseName),
sql.EscapeName(tableName),
this.migrationContext.TableEngine,
Expand Down Expand Up @@ -949,14 +950,14 @@ func (this *Applier) AtomicCutOverMagicLock(sessionIdChan chan int64, tableLocke
}()

var sessionId int64
if err := tx.QueryRow(`select connection_id()`).Scan(&sessionId); err != nil {
if err := tx.QueryRow(`select /* gh-ost */ connection_id()`).Scan(&sessionId); err != nil {
tableLocked <- err
return err
}
sessionIdChan <- sessionId

lockResult := 0
query := `select get_lock(?, 0)`
query := `select /* gh-ost */ get_lock(?, 0)`
lockName := this.GetSessionLockName(sessionId)
this.migrationContext.Log.Infof("Grabbing voluntary lock: %s", lockName)
if err := tx.QueryRow(query, lockName).Scan(&lockResult); err != nil || lockResult != 1 {
Expand All @@ -967,7 +968,7 @@ func (this *Applier) AtomicCutOverMagicLock(sessionIdChan chan int64, tableLocke

tableLockTimeoutSeconds := this.migrationContext.CutOverLockTimeoutSeconds * 2
this.migrationContext.Log.Infof("Setting LOCK timeout as %d seconds", tableLockTimeoutSeconds)
query = fmt.Sprintf(`set session lock_wait_timeout:=%d`, tableLockTimeoutSeconds)
query = fmt.Sprintf(`set /* gh-ost */ session lock_wait_timeout:=%d`, tableLockTimeoutSeconds)
if _, err := tx.Exec(query); err != nil {
tableLocked <- err
return err
Expand Down Expand Up @@ -1026,7 +1027,7 @@ func (this *Applier) AtomicCutOverMagicLock(sessionIdChan chan int64, tableLocke
sql.EscapeName(this.migrationContext.DatabaseName),
sql.EscapeName(this.migrationContext.GetOldTableName()),
)
query = `unlock tables`
query = `unlock /* gh-ost */ tables`
if _, err := tx.Exec(query); err != nil {
tableUnlocked <- err
return this.migrationContext.Log.Errore(err)
Expand All @@ -1048,13 +1049,13 @@ func (this *Applier) AtomicCutoverRename(sessionIdChan chan int64, tablesRenamed
tablesRenamed <- fmt.Errorf("Unexpected error in AtomicCutoverRename(), injected to release blocking channel reads")
}()
var sessionId int64
if err := tx.QueryRow(`select connection_id()`).Scan(&sessionId); err != nil {
if err := tx.QueryRow(`select /* gh-ost */ connection_id()`).Scan(&sessionId); err != nil {
return err
}
sessionIdChan <- sessionId

this.migrationContext.Log.Infof("Setting RENAME timeout as %d seconds", this.migrationContext.CutOverLockTimeoutSeconds)
query := fmt.Sprintf(`set session lock_wait_timeout:=%d`, this.migrationContext.CutOverLockTimeoutSeconds)
query := fmt.Sprintf(`set /* gh-ost */ session lock_wait_timeout:=%d`, this.migrationContext.CutOverLockTimeoutSeconds)
if _, err := tx.Exec(query); err != nil {
return err
}
Expand All @@ -1080,7 +1081,7 @@ func (this *Applier) AtomicCutoverRename(sessionIdChan chan int64, tablesRenamed
}

func (this *Applier) ShowStatusVariable(variableName string) (result int64, err error) {
query := fmt.Sprintf(`show global status like '%s'`, variableName)
query := fmt.Sprintf(`show /* gh-ost */ global status like '%s'`, variableName)
if err := this.db.QueryRow(query).Scan(&variableName, &result); err != nil {
return 0, err
}
Expand Down Expand Up @@ -1150,7 +1151,7 @@ func (this *Applier) ApplyDMLEventQueries(dmlEvents [](*binlog.BinlogDMLEvent))
return err
}

sessionQuery := "SET SESSION time_zone = '+00:00'"
sessionQuery := "SET /* gh-ost */ SESSION time_zone = '+00:00'"
Copy link
Contributor

@shaohk shaohk Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timvaillancourt Does the time zone set here need to be configured as this.migrationContext.ApplierTimeZone? I noticed that other sessions are using this.migrationContext.ApplierTimeZone for setting the time zone, but here it is set to +00:00. Should this be adjusted?

Copy link
Collaborator Author

@timvaillancourt timvaillancourt Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shaohk good question, that sounds more correct than +00:00 🤔

Based on the soak testing this logic has seen I don't think this causes an integrity problem, however. I think that may be because binlog row events contain the values from that primary/master (including dates/times), so it probably doesn't matter what timezone the applier session uses here. I believe gh-ost is using the decoded binlog row event values only

Fewer "what if?" questions is a good thing though, so I would still welcome this change if it passes integration testing at GitHub

sessionQuery = fmt.Sprintf("%s, %s", sessionQuery, this.generateSqlModeQuery())

if _, err := tx.Exec(sessionQuery); err != nil {
Expand Down
34 changes: 17 additions & 17 deletions go/logic/applier_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,13 +114,12 @@ func TestApplierBuildDMLEventQuery(t *testing.T) {
res := applier.buildDMLEventQuery(binlogEvent)
test.S(t).ExpectEquals(len(res), 1)
test.S(t).ExpectNil(res[0].err)
test.S(t).ExpectEquals(strings.TrimSpace(res[0].query),
`delete /* gh-ost `+"`test`.`_test_gho`"+` */
from
`+"`test`.`_test_gho`"+`
where
((`+"`id`"+` = ?) and (`+"`item_id`"+` = ?))`)

test.S(t).ExpectEquals(strings.TrimSpace(res[0].query), `delete /* gh-ost `+"`test`.`_test_gho`"+` */
from
`+"`test`.`_test_gho`"+`
where
((`+"`id`"+` = ?) and (`+"`item_id`"+` = ?))`,
)
test.S(t).ExpectEquals(len(res[0].args), 2)
test.S(t).ExpectEquals(res[0].args[0], 123456)
test.S(t).ExpectEquals(res[0].args[1], 42)
Expand All @@ -136,11 +135,12 @@ func TestApplierBuildDMLEventQuery(t *testing.T) {
test.S(t).ExpectEquals(len(res), 1)
test.S(t).ExpectNil(res[0].err)
test.S(t).ExpectEquals(strings.TrimSpace(res[0].query),
`replace /* gh-ost `+"`test`.`_test_gho`"+` */ into
`+"`test`.`_test_gho`"+`
`+"(`id`, `item_id`)"+`
values
(?, ?)`)
`replace /* gh-ost `+"`test`.`_test_gho`"+` */
into
`+"`test`.`_test_gho`"+`
`+"(`id`, `item_id`)"+`
values
(?, ?)`)
test.S(t).ExpectEquals(len(res[0].args), 2)
test.S(t).ExpectEquals(res[0].args[0], 123456)
test.S(t).ExpectEquals(res[0].args[1], 42)
Expand All @@ -158,11 +158,11 @@ func TestApplierBuildDMLEventQuery(t *testing.T) {
test.S(t).ExpectNil(res[0].err)
test.S(t).ExpectEquals(strings.TrimSpace(res[0].query),
`update /* gh-ost `+"`test`.`_test_gho`"+` */
`+"`test`.`_test_gho`"+`
set
`+"`id`"+`=?, `+"`item_id`"+`=?
where
((`+"`id`"+` = ?) and (`+"`item_id`"+` = ?))`)
`+"`test`.`_test_gho`"+`
set
`+"`id`"+`=?, `+"`item_id`"+`=?
where
((`+"`id`"+` = ?) and (`+"`item_id`"+` = ?))`)
test.S(t).ExpectEquals(len(res[0].args), 4)
test.S(t).ExpectEquals(res[0].args[0], 123456)
test.S(t).ExpectEquals(res[0].args[1], 42)
Expand Down
Loading
Loading