sql: use background QoS for atomic COPY#115674
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
rafiss
left a comment
There was a problem hiding this comment.
nice find - have you thought of how we could test this?
We recently merged a change to make COPY use "background" QoS by default. However, that change was incomplete - it only made it so that we use the "background" QoS only whenever a new txn is started by the copy state machine, and we forgot to make the corresponding update for the initial txn created outside of the state machine, if we're in an implicit txn. This is now fixed. Note that this initial txn is only used for "atomic" COPY because for non-atomic we always create a fresh txn before each batch. This commit also adjusts an existing test to verify that the expected QoS is used for all implicit txns. Release note: None
yuzefovich
left a comment
There was a problem hiding this comment.
Good point - adjusted an existing test to do that. Working on this also made me realize that this PR fixes only the case of atomic COPY - for non-atomic COPY even the initial batch is handled by the txn with background QoS because we call preparePlannerForCopy before each batch, which commits the original txn with default QoS and creates a new one with background QoS. Thanks for the nudge!
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @michae2)
|
Friendly ping @rafiss |
rafiss
left a comment
There was a problem hiding this comment.
the new test is great, thanks
Reviewed 2 of 3 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status:
|
TFTR! bors r+ |
|
Build succeeded: |
We recently merged a change to make COPY use "background" QoS by default. However, that change was incomplete - it only made it so that we use the "background" QoS only whenever a new txn is started by the copy state machine, and we forgot to make the corresponding update for the initial txn created outside of the state machine, if we're in an implicit txn. This is now fixed.
Note that this initial txn is only used for "atomic" COPY because for non-atomic we always create a fresh txn before each batch.
This commit also adjusts an existing test to verify that the expected QoS is used for all implicit txns.
Epic: None
Release note: None