Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Cancel any row count queries before attempting to cut over #846
Conversation
Closes #830. Switches from using `QueryRow` to `QueryRowContext`, and stores a context.CancelFunc in the migration context, which is called to halt any running row count query before beginning the cut over.
|
Thank you for this important fix! To the best of my understanding the termination of the rowcount query is not fully implemented in this PR, see comments inline. |
| @@ -401,6 +406,10 @@ func (this *Migrator) Migrate() (err error) { | |||
| } | |||
| this.printStatus(ForcePrintStatusRule) | |||
|
|
|||
| if this.migrationContext.CountTableRowsCancelFunc != nil { | |||
shlomi-noach
Jun 28, 2020
Contributor
Needs to be thread protected. Potential concurrent change: https://github.com/github/gh-ost/pull/846/files#diff-5aa50dde7ca1baf7a819eecf740ea220R543
| default: | ||
| // row count query finished. nil out the cancel func, so the main migration thread | ||
| // doesn't bother calling it after row copy is done. | ||
| this.migrationContext.CountTableRowsCancelFunc = nil |
shlomi-noach
Jun 28, 2020
Contributor
Needs to be thread protected. Potential concurrent read: https://github.com/github/gh-ost/pull/846/files#diff-e2990679ccfa83d6fc15414958333bcbR409
| atomic.StoreInt64(&this.migrationContext.CountingRowsFlag, 1) | ||
| defer atomic.StoreInt64(&this.migrationContext.CountingRowsFlag, 0) | ||
|
|
||
| log.Infof("As instructed, I'm issuing a SELECT COUNT(*) on the table. This may take a while") | ||
|
|
||
| query := fmt.Sprintf(`select /* gh-ost */ count(*) as rows from %s.%s`, sql.EscapeName(this.migrationContext.DatabaseName), sql.EscapeName(this.migrationContext.OriginalTableName)) | ||
| var rowsEstimate int64 | ||
| if err := this.db.QueryRow(query).Scan(&rowsEstimate); err != nil { | ||
| if err := this.db.QueryRowContext(ctx, query).Scan(&rowsEstimate); err != nil { |
shlomi-noach
Jun 28, 2020
Contributor
To the best of my understanding, the go-sql-driver does not actually kill the query when context is cancelled, it is on the user to issue a KILL query. In which case we should first obtain a Connection so that we can get its connection_id, which we can then use to kill it.
ajm188
Jun 28, 2020
Author
Contributor
You're right! I regret this statement
Update on the above: I smartened up and read the docs instead of the code, which say go-sql-driver does support cancelling queries via contexts
I think the row count has to be pushed into a goroutine then, so CountTableRows can simultaneously wait on it and on a signal to kill the count query.
* Explicitly grab a connection to run the count, store its connection id * When the query context is canceled, run a `KILL QUERY ?` on that connection id
Closes #830. Switches from using
QueryRowtoQueryRowContext, andstores a context.CancelFunc in the migration context, which is called to
halt any running row count query before beginning the cut over.
Note: I'm 99% sure
database/sqldoes what I think it should do here. I wanted to open this now in case anyone else happens to know off-hand, but in the meantime I'll keep reading through the go stdlib to convince myself this is right.Update on the above: I smartened up and read the docs instead of the code, which say
go-sql-driverdoes support cancelling queries via contextsscript/cibuildreturns with no formatting errors, build errors or unit test errors.