Cancel any row count queries before attempting to cut over #846
Conversation
This file contains hidden or 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
Closes github#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.
shlomi-noach
suggested changes
Jun 28, 2020
Contributor
shlomi-noach
left a comment
shlomi-noach
left a comment
There was a problem hiding this comment.
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.
added 3 commits
June 28, 2020 11:04
* 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
timvaillancourt
approved these changes
Jun 4, 2022
Collaborator
|
@ajm188 this PR makes a lot of sense to me. Thanks a lot! Do you mind rebasing the PR? I think the main change is the logger 🙏 |
Collaborator
Contributor
Author
awesome, thanks for taking care of it! |
Contributor
Author
|
@timvaillancourt any update on getting this merged? 🤞 |
ghost
approved these changes
Jul 6, 2022
ghost
pushed a commit
that referenced
this pull request
Jul 7, 2022
* Cancel any row count queries before attempting to cut over 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. * Make it threadsafe * Kill the count query on the database side as well * 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 * Rewrite these to use the threadsafe functions, stop exporting the cancel func * Update logger * Update logger Co-authored-by: Tim Vaillancourt <timvaillancourt@github.com> Co-authored-by: Tim Vaillancourt <tim@timvaillancourt.com> Co-authored-by: dm-2 <45519614+dm-2@users.noreply.github.com>
ghost
mentioned this pull request
Closed
ghost
pushed a commit
that referenced
this pull request
Jul 7, 2022
* Cancel any row count queries before attempting to cut over 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. * Make it threadsafe * Kill the count query on the database side as well * 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 * Rewrite these to use the threadsafe functions, stop exporting the cancel func * Update logger * Update logger Co-authored-by: Tim Vaillancourt <timvaillancourt@github.com> Co-authored-by: Tim Vaillancourt <tim@timvaillancourt.com> Co-authored-by: dm-2 <45519614+dm-2@users.noreply.github.com>
2 tasks
RainbowDashy
pushed a commit
to RainbowDashy/gh-ost
that referenced
this pull request
Jul 13, 2022
* Cancel any row count queries before attempting to cut over Closes github#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. * Make it threadsafe * Kill the count query on the database side as well * 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 * Rewrite these to use the threadsafe functions, stop exporting the cancel func * Update logger * Update logger Co-authored-by: Tim Vaillancourt <timvaillancourt@github.com> Co-authored-by: Tim Vaillancourt <tim@timvaillancourt.com> Co-authored-by: dm-2 <45519614+dm-2@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
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.