-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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.
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
sql: step the transaction before auto-commit #86153
Conversation
We want the pre-commit validation to see the side-effects of all previous statements. If we don't step the transaction, we won't see writes which occurred in the last statement. The shocking thing is that this all worked before this change. The reason it worked was that the internal executor usage that happened as a part of logging every schema change resulted in the txn being stepped. That is a separate, and more complex bug to fix. It will be described and addressed separately. Fixes cockroachdb#86132 Release note: None
aa6a373
to
f28caf3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but you need to update the expected output for the enums
logic test.
needed before #86174 can merge |
Doesn't need a rewrite, was a flake fixed by #86276 |
so this is good to go? |
Yeah, here goes. bors r+ |
Build failed (retrying...): |
I think this is bound to fail due to |
bors r- |
Canceled. |
I dug into this more. It turns out that test is flakey. It does not look at the cockroach/pkg/sql/telemetry_logging_test.go Lines 348 to 350 in fd1b51f
#86118 bors r+ |
Build succeeded: |
In cockroachdb#86153 we added logic to step the transaction before auto-commit. This was insufficient to resolve cockroachdb#86132 because we did not step the transaction before committing an explicit transaction. This PR addresses that by moving the step logic to a shared code path. It also adds a test which previously failed. Fixes cockroachdb#86132 Release justification: bug fix Release note: None
In cockroachdb#86153 we added logic to step the transaction before auto-commit. This was insufficient to resolve cockroachdb#86132 because we did not step the transaction before committing an explicit transaction. This PR addresses that by moving the step logic to a shared code path. It also adds a test which previously failed. Fixes cockroachdb#86132 Release justification: bug fix Release note: None
86429: roachtest: add a test case for the decom benchmark r=lidorcarmel a=lidorcarmel and also enable allocator vmodule for this roachtest. Release justification: a small test-only change. Release note: None 86443: opt: reduce allocations in OrderingSet.LongestCommonPrefix r=mgartner a=mgartner Previously, the `*OrderingChoice` returned by `LongestCommonPrefix` was allocated on the heap (except in the case where the ordering set contained a ordering choice that implies the given ordering). Now, `LongestCommonPrefix` returns a non-pointer type and avoids these allocations. Release justification: This is a minor change that improves performance. Release note: None 86445: storage: disable `pebbleIterator` range key block filtering r=jbowens a=erikgrinaker **storage: disable `pebbleIterator` range key block filtering** This patch disables TBI filtering of MVCC range keys blocks via `MinTimestampHint` and `MaxTimestampHint`, due to complications with `MVCCIncrementalIterator`. Specifically, the TBI and regular iterators would have a different view of the range key fragmentation, which could cause it to skip past range key fragments. The necessary seeks and other processing to handle this would likely negate the benefit of the filtering. This causes a minor regression when range keys are present, because the TBI optimization can currently get "stuck" when seeking into range keys. Previously, these range keys would be filtered out by the TBI. A separate commit will address this. ``` name old time/op new time/op delta CatchUpScan/mixed-case/withDiff=false/perc=0.00/numRangeKeys=0-24 804ms ± 1% 798ms ± 1% ~ (p=0.151 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=0.00/numRangeKeys=1-24 990ms ± 1% 972ms ± 1% -1.84% (p=0.008 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=0.00/numRangeKeys=100-24 991ms ± 0% 984ms ± 1% -0.73% (p=0.032 n=4+5) CatchUpScan/mixed-case/withDiff=false/perc=50.00/numRangeKeys=0-24 676ms ± 0% 689ms ± 1% +2.03% (p=0.008 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=50.00/numRangeKeys=1-24 859ms ± 0% 867ms ± 1% +0.95% (p=0.008 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=50.00/numRangeKeys=100-24 868ms ± 0% 877ms ± 0% +1.06% (p=0.008 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=95.00/numRangeKeys=0-24 181ms ± 0% 183ms ± 1% +0.90% (p=0.016 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=95.00/numRangeKeys=1-24 204ms ± 1% 206ms ± 1% +1.20% (p=0.008 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=95.00/numRangeKeys=100-24 294ms ± 1% 323ms ± 4% +9.87% (p=0.008 n=5+5) ``` Resolves #86260. Release justification: bug fixes and low-risk updates to new functionality Release note: None **storage: prevent `MVCCIncrementalIterator` TBI stuck on range key** This patch prevents the internal TBI used in `MVCCIncrementalIterator` from getting prematurely stuck on an MVCC range key covering a seek key. Consider the following visible keys for the TBI (time range 3-5) and regular iterator: Iterator: `[a-f)`@5`,` `a@4`, `c@1`, and `e@4` TBI: `[a-f)`@5`,` `a@4`, `e@4`. If the iterator is positioned on `c@1` (outside the time bounds), and the TBI is seeked to `c` to find newer keys, the the TBI will get stuck on the range key `[a-f)`@5`` even though it has already been emitted, preventing the TBI from being used at all. This patch moves the TBI forward to `e@4` in this case, and similarly when seeking iter ahead. The effect on performance appears rather marginal though. ``` name old time/op new time/op delta CatchUpScan/mixed-case/withDiff=false/perc=0.00/numRangeKeys=0-24 798ms ± 1% 805ms ± 1% +0.93% (p=0.032 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=0.00/numRangeKeys=1-24 972ms ± 1% 983ms ± 0% +1.15% (p=0.016 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=0.00/numRangeKeys=100-24 984ms ± 1% 995ms ± 0% +1.09% (p=0.016 n=5+4) CatchUpScan/mixed-case/withDiff=false/perc=50.00/numRangeKeys=0-24 689ms ± 1% 681ms ± 0% -1.28% (p=0.008 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=50.00/numRangeKeys=1-24 867ms ± 1% 854ms ± 0% -1.57% (p=0.008 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=50.00/numRangeKeys=100-24 877ms ± 0% 869ms ± 1% ~ (p=0.056 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=95.00/numRangeKeys=0-24 183ms ± 1% 179ms ± 0% -1.90% (p=0.008 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=95.00/numRangeKeys=1-24 206ms ± 1% 204ms ± 1% -1.10% (p=0.016 n=5+5) CatchUpScan/mixed-case/withDiff=false/perc=95.00/numRangeKeys=100-24 323ms ± 4% 315ms ± 1% -2.37% (p=0.016 n=5+5) ``` Release justification: bug fixes and low-risk updates to new functionality Release note: None 86461: sql: step the sql transaction before commit r=knz a=ajwerner In #86153 we added logic to step the transaction before auto-commit. This was insufficient to resolve #86132 because we did not step the transaction before committing an explicit transaction. This PR addresses that by moving the step logic to a shared code path. It also adds a test which previously failed. Fixes #86132 Release justification: bug fix Release note: None 86463: changefeedccl: clean up logic for determining dropped targets r=yuzefovich a=yuzefovich Release justification: low-risk cleanup. Release note: None Co-authored-by: Lidor Carmel <[email protected]> Co-authored-by: Marcus Gartner <[email protected]> Co-authored-by: Erik Grinaker <[email protected]> Co-authored-by: Andrew Werner <[email protected]> Co-authored-by: Yahor Yuzefovich <[email protected]>
We want the pre-commit validation to see the side-effects of all previous
statements. If we don't step the transaction, we won't see writes which
occurred in the last statement. The shocking thing is that this all worked
before this change. The reason it worked was that the internal executor usage
that happened as a part of logging every schema change resulted in the txn
being stepped. That is a separate, and more complex bug to fix. It will be
described and addressed separately.
Fixes #86132
Release justification: Important bug fix
Release note: None