release-20.2: coldata: fix updating offsets of bytes in Batch.SetLength

[yuzefovich]

Backport 1/2 commits from #59028.

/cc @cockroachdb/release

---

In SetLength method we are maintaining the invariant of Bytes
vectors that the offsets are non-decreasing sequences. Previously, this
was done incorrectly when a selection vector is present on the batch
which could lead to out of bounds errors (caught by our panic-catcher)
some time later. This is now fixed by correctly paying attention to the
selection vector.

I neither can easily come up with an example query that would trigger
this condition nor can I prove that it won't occur, but I think we have
seen a single sentry report that could be explained by this bug, so I
think it's worth backporting.

Fixes: #57297.

Release note (bug fix): Previously, CockroachDB could encounter an
internal error when executing queries with BYTES or STRING types via the
vectorized engine in rare circumstances, and now this is fixed.

[cockroach-teamcity]

This change is 

[asubiotto]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)

---

pkg/col/coldata/batch.go, line 252 at r1 (raw file):

		// by the batch.
		maxIdx := length - 1
		if m.useSel && m.sel[length-1] > maxIdx {

Can we be extra-safe here and also check m.sel != nil && len(m.sel) >= length before indexing?

---

pkg/col/coldata/batch.go, line 257 at r1 (raw file):

			//
			// This assumption is only enforced by the invariantsChecker
			// starting from 21.1 branches, so we have a "safe" conditional to

Could you point me to it?

[yuzefovich]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto)

---

pkg/col/coldata/batch.go, line 252 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Can we be extra-safe here and also check m.sel != nil && len(m.sel) >= length before indexing?

I think that would be an overkill. If m.useSel is true, then the batch must have a selection vector with length of at least length, since otherwise this batch is broken anyway, so it doesn't matter whether we crash when setting the length on it or some time later when reading from it.

---

pkg/col/coldata/batch.go, line 257 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Could you point me to it?

cockroach/pkg/sql/colexec/invariants_checker.go

Lines 55 to 64 in 7df5918

	if sel := b.Selection(); sel != nil {
	for i := 1; i < n; i++ {
	if sel[i] <= sel[i-1] {
	colexecerror.InternalError(errors.AssertionFailedf(
	"unexpectedly selection vector is not an increasing sequence "+
	"at position %d: %v", i, sel[:n],
	))
	}
	}
	}

[yuzefovich]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto)

---

pkg/col/coldata/batch.go, line 257 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

cockroach/pkg/sql/colexec/invariants_checker.go

Lines 49 to 54 in 7df5918

	for colIdx := 0; colIdx < b.Width(); colIdx++ {
	v := b.ColVec(colIdx)
	if v.CanonicalTypeFamily() == types.BytesFamily {
	v.Bytes().AssertOffsetsAreNonDecreasing(n)
	}
	}

Oops, wrong selected lines. Correct ones:

cockroach/pkg/sql/colexec/invariants_checker.go

Lines 55 to 64 in 7df5918

	if sel := b.Selection(); sel != nil {
	for i := 1; i < n; i++ {
	if sel[i] <= sel[i-1] {
	colexecerror.InternalError(errors.AssertionFailedf(
	"unexpectedly selection vector is not an increasing sequence "+
	"at position %d: %v", i, sel[:n],
	))
	}
	}
	}

[asubiotto]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto and @yuzefovich)

---

pkg/col/coldata/batch.go, line 257 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Oops, wrong selected lines. Correct ones:

cockroach/pkg/sql/colexec/invariants_checker.go

Lines 55 to 64 in 7df5918

	if sel := b.Selection(); sel != nil {
	for i := 1; i < n; i++ {
	if sel[i] <= sel[i-1] {
	colexecerror.InternalError(errors.AssertionFailedf(
	"unexpectedly selection vector is not an increasing sequence "+
	"at position %d: %v", i, sel[:n],
	))
	}
	}
	}

Sorry, I meant the "safe conditional" you mention in this branch. I guess I'm just worried that this invariant is not checked in this branch. What about a check to ensure that if:

if maxIdx < length {
    maxIdx = length
}

[yuzefovich]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto)

---

pkg/col/coldata/batch.go, line 257 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Sorry, I meant the "safe conditional" you mention in this branch. I guess I'm just worried that this invariant is not checked in this branch. What about a check to ensure that if:

if maxIdx < length {
    maxIdx = length
}

Yeah, I share your concern, that's why I added m.sel[length-1] > maxIdx part above (which is - I think - exactly what you're suggesting, albeit in an "inverse" form).

[asubiotto]

Reviewed 1 of 1 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @asubiotto and @yuzefovich)

---

pkg/col/coldata/batch.go, line 257 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Yeah, I share your concern, that's why I added m.sel[length-1] > maxIdx part above (which is - I think - exactly what you're suggesting, albeit in an "inverse" form).

Carry on 😂