exec: modify tests to catch bad selection vector access#40305
exec: modify tests to catch bad selection vector access#40305craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
yuzefovich
left a comment
There was a problem hiding this comment.
Nice catch!
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis and @rafiss)
pkg/sql/exec/utils_test.go, line 242 at r1 (raw file):
s.selection = make([]uint16, coldata.BatchSize) for i := range s.selection {
I think this for loop initialization can now be removed.
pkg/sql/exec/utils_test.go, line 269 at r1 (raw file):
if s.useSel { for i := range s.selection {
I'd initialize this s.selection up to batchSize here, and then move the for loop for the unused elements right after this for loop.
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/exec/utils_test.go, line 269 at r1 (raw file):
Previously, yuzefovich wrote…
I'd initialize this
s.selectionup tobatchSizehere, and then move theforloop for the unused elements right after thisforloop.
Actually, never mind, we're using full s.selection in the shuffle step.
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/exec/utils_test.go, line 242 at r1 (raw file):
Previously, yuzefovich wrote…
I think this
forloop initialization can now be removed.
It seems like we can't, since s.selection is used further below, even when useSel is false. I added a comment.
The runTests helper will now cause a panic if a vectorized operator tries to access a part of the selection vector that is out of bounds. This identified bugs in the projection operator. Release note: None
ae7caa8 to
fa2ae02
Compare
|
thanks for the review! bors r+ |
40208: distsql: add disk spilling to lookup joiner r=solongordon a=solongordon In lookup joins on partial index keys, there is no limit on how many rows might be returned by any particular lookup, so the joinreader may be buffering an unbounded number of rows into memory. I changed joinreader to use a disk-backed row container rather than just storing the rows in memory with no accounting. Fixes #39044 Release note (bug fix): Lookup joins now spill to disk if the index lookups return more rows than can be stored in memory. 40284: storage: issue swaps on AllocatorConsiderRebalance r=nvanbenschoten a=tbg Change the rebalancing code so that it not only looks up a new replica to add, but also picks one to remove. Both actions are then given to a ChangeReplicas invocation which will carry it out atomically as long as that feature is enabled. Release note (bug fix): Replicas can now be moved between stores without entering an intermediate configuration that violates the zone constraints. Violations may still occur during zone config changes, decommissioning, and in the presence of dead nodes (NB: the remainder be addressed in a future change, so merge the corresponding release note) 40300: store: pull updateMVCCGauges out of StoreMetrics lock, use atomics r=nvanbenschoten a=nvanbenschoten The operations it performs are already atomic, so we can use atomic add instructions to avoid any critical section. This was responsible for 8.15% of mutex contention on a YCSB run. The change also removes MVCCStats from the `storeMetrics` interface, which addresses a long-standing TODO. 40301: roachtest: Deflake clock jump test r=tbg a=bdarnell These tests perform various clock jumps, then reverse them. The reverse can cause a crash even if the original jump did not. Add some sleeps to make things more deterministic and improve the recovery process at the end of the test. Fixes #38723 Release note: None 40305: exec: modify tests to catch bad selection vector access r=rafiss a=rafiss The runTests helper will now cause a panic if a vectorized operator tries to access a part of the selection vector that is out of bounds. This identified bugs in the projection operator. Release note: None Co-authored-by: Solon Gordon <solon@cockroachlabs.com> Co-authored-by: Tobias Schottdorf <tobias.schottdorf@gmail.com> Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com> Co-authored-by: Ben Darnell <ben@cockroachlabs.com> Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
| if s.useSel { | ||
| for i := range s.selection { | ||
| s.selection[i] = uint16(i) | ||
| } |
There was a problem hiding this comment.
how come we needed to add this OOC?
There was a problem hiding this comment.
the slice is reused, so if we didn't do this, the next call to Next would shuffle the "bad" elements of the selection vector into the "good" part of the slice.
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/exec/utils_test.go, line 271 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
the slice is reused, so if we didn't do this, the next call to
Nextwould shuffle the "bad" elements of the selection vector into the "good" part of the slice.
It's the same thing as ResetInternalBatch.
Build succeeded |
The runTests helper will now cause a panic if a vectorized operator
tries to access a part of the selection vector that is out of bounds.
This identified bugs in the projection operator.
Release note: None