Skip to content

Commit

Permalink
colexecop: reset the internal batch in a test operator
Browse files Browse the repository at this point in the history
This commit makes a test operator that "owns" a batch to properly reset
the batch on each `Next` call. This is the expectation that we forgot to
follow.

This was discovered when running `BenchmarkLikeOps` which uses a Bytes
vector. Due to the way `Bytes.Copy` is implemented, the Bytes vector's
buffer could grow arbitrarily large because the test operator never
reset the vector.

Release note: None
  • Loading branch information
yuzefovich committed Mar 30, 2022
1 parent d5afa87 commit 699284b
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 1 deletion.
8 changes: 8 additions & 0 deletions pkg/col/coldata/bytes.go
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,14 @@ func (b *Bytes) copyElements(srcElementsToCopy []element, src *Bytes, destIdx in
// Optimize copying of the elements by copying all of them directly into the
// destination. This way all inlined values become correctly set, and we
// only need to set the non-inlined values separately.
//
// Note that this behavior results in losing the references to the old
// non-inlined values, even if they could be reused. If Bytes is not Reset,
// then that unused space in Bytes.buffer can accumulate. However, checking
// whether there are old non-inlined values with non-zero capacity leads to
// performance regressions, and in the production code we do reset the Bytes
// in all cases, so we accept this poor behavior in such a hypothetical /
// test-only scenario. See #78703 for more details.
copy(destElements, srcElementsToCopy)
// Early bounds checks.
_ = destElements[len(srcElementsToCopy)-1]
Expand Down
3 changes: 2 additions & 1 deletion pkg/sql/colexecop/testutils.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,9 @@ func (s *RepeatableBatchSource) Next() coldata.Batch {
if s.batchesToReturn != 0 && s.batchesReturned > s.batchesToReturn {
return coldata.ZeroBatch
}
s.output.SetSelection(s.sel != nil)
s.output.ResetInternalBatch()
if s.sel != nil {
s.output.SetSelection(true)
copy(s.output.Selection()[:s.batchLen], s.sel[:s.batchLen])
}
for i, colVec := range s.colVecs {
Expand Down

0 comments on commit 699284b

Please sign in to comment.