sql: make COPY TO/FROM respect timeouts and cancellation, and appear in SHOW QUERIES#97808
Merged
craig[bot] merged 2 commits intocockroachdb:masterfrom Mar 1, 2023
Merged
Conversation
Member
|
This change is |
422a954 to
85976ec
Compare
Contributor
|
cc @cucaroach in case this influences any work you're doing on copy statement bundles |
otan
reviewed
Feb 28, 2023
Contributor
There was a problem hiding this comment.
did a first pass: some tests look like they need some fixing.
i also have a minor concern about how this affects retries - COPY FROM / TO should never retry (given it directly digests / spits out messages on the wire without buffering them). do you know if that is still valid / a test case already covers them?
Collaborator
Author
|
yes, there are tests. see |
cucaroach
reviewed
Feb 28, 2023
85976ec to
7086a95
Compare
Using the conn executor state machine allows a lot of bespoke logic to be removed. (If we were to remove the copy_from_atomic_enabled setting and make it always true, we could remove even more custom logic.) This then makes it easy to hook up to cancellation and introspection. Release note (bug fix): The COPY FROM command now respects the statement_timeout and transaction_timeout settings. Release note (bug fix): The COPY FROM command now appears in the results of SHOW QUERIES.
This commit simplifies the conn executor state transitions a bit, and adds in timers for stmt/txn timeouts analogously to how they were for COPY FROM. No release note since COPY TO is new. Release note: None
7086a95 to
78e6280
Compare
Collaborator
Author
|
i've fixed those test failures; the changes were pretty minor |
otan
approved these changes
Mar 1, 2023
Collaborator
Author
|
tftr! bors r=otan |
Contributor
|
Build succeeded: |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
informs #94194
sql: use conn executor state machine for COPY FROM
Using the conn executor state machine allows a lot of bespoke logic to
be removed. (If we were to remove the copy_from_atomic_enabled setting
and make it always true, we could remove even more custom logic.)
This then makes it easy to hook up to cancellation and introspection.
Release note (bug fix): The COPY FROM command now respects the
statement_timeout and transaction_timeout settings.
Release note (bug fix): The COPY FROM command now appears in the results
of SHOW QUERIES.
sql: make stmt/txn timeout apply to COPY TO
This commit simplifies the conn executor state transitions a bit, and
adds in timers for stmt/txn timeouts analogously to how they were for
COPY FROM.
No release note since COPY TO is new.