coldata: expose a modifier for batchSize variable

[yuzefovich]

This commit exposes a setter for batchSize private variable to be
modified in tests and runs all tests in sql/colexec with a random
batch size in [3, 4096] range.

The tests in several places needed to be adjusted because their
assumptions might no longer hold when batch size is modified.

Fixes: #40791.

Release note: None

[cockroach-teamcity]

This change is 

[yuzefovich]

Not requesting a review for now since I want to see what the fallout is first. We probably will want to limit the config to two batch sizes - the default (1024) and a random (I think it must be greater than 1).

[yuzefovich]

This didn't discover any new bugs, but #42816 would have been caught by this change when batchSize is set to 3.

Also, I'm not sure whether we want to randomize the second batch size or just pick 3 (smallest I've tried).

[asubiotto]

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

---

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

const (
	// MinBatchSize is the minimum acceptable size of batches.
	MinBatchSize = 3

What happens if we make this 1?

---

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

	MinBatchSize = 3
	// MaxBatchSize is the maximum acceptable size of batches.
	MaxBatchSize = 1024

Do we want to try having a max batch size > 1024?

---

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

// SetBatchSizeForTests modifies batchSize variable. It should only be used in
// tests.
func SetBatchSizeForTests(newBatchSize uint16) {

I wonder if there's a way to only make this method available during tests. Does putting it in a _test.go file work?

---

pkg/sql/colexec/main_test.go, line 57 at r1 (raw file):

			fmt.Printf("coldata.BatchSize() is set to %d\n", batchSize)
			coldata.SetBatchSizeForTests(batchSize)
			if code := m.Run(); code != 0 {

Do we want to run these tests twice? How does the runtime increase? I think we should just run them with a random batch size that we print out in the same way that the random seed is printed out using randutil.

---

pkg/sql/colexec/routers_test.go, line 477 at r1 (raw file):

	if _, ok := expectedNumVals[batchSize]; !ok {
		// We do not have a precomputed expectedNumVals for this batch size, so we

But then we have a very low probability of actually running this test. We might want to override the batch size here by getting the batch size, calling SetBatchSize ourselves with a constant value, then resetting it with a defer.

[yuzefovich]

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

---

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

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

What happens if we make this 1?

There are multiple unit tests that have baked-in assumption of a "big" batch size, those would need to be changed. Also, I feel like some operators might have that assumption, and they might get stuck in an infinite loop or something (or maybe not). I'm not sure how much of a change that would be, but I think we will benefit from getting this PR in with 3 as the minimum and then during stabilization period possibly pushing it all the way down to 1.

---

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

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

Do we want to try having a max batch size > 1024?

I tried with 2048, 4096, and 8192, and the tests passed (I needed to do a minor tweak to one aggregator random test because there was uint16 overflow). I do feel that unless we decide to use bigger batch sizes in production, there is not much benefit from testing with larger sizes - most of our unit tests have very little data, so if we increase the batch size, the likelihood of uncovering a bug doesn't seem to increase (nor to decrease). Running randomized unit tests might get slightly better coverage at the expense of longer runtime. I think that pushing the minimum as low as possible is beneficial, not sure about the maximum though.

---

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

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

I wonder if there's a way to only make this method available during tests. Does putting it in a _test.go file work?

Unfortunately, that doesn't work. If we move the setter method in _test file, it is only visible within coldata package (not even coldata_test); if we move it to coldata_test package, then it is visible only that package.

---

pkg/sql/colexec/main_test.go, line 57 at r1 (raw file):

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

Do we want to run these tests twice? How does the runtime increase? I think we should just run them with a random batch size that we print out in the same way that the random seed is printed out using randutil.

The runtime increases more or less linearly (there are randomized unit tests which depend on the batch size, so with the smaller sizes, those run faster). The whole test suite of colexec package takes 10 seconds or so on my laptop, I think we could run it with the default batch size and a random one, but I'm not sure whether that would give us any benefits. Switched to only random batch size.

---

pkg/sql/colexec/routers_test.go, line 477 at r1 (raw file):

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

But then we have a very low probability of actually running this test. We might want to override the batch size here by getting the batch size, calling SetBatchSize ourselves with a constant value, then resetting it with a defer.

True, I was imagining this being useful if we always run the tests with the default config. Changed.

[asubiotto]

Reviewed 5 of 5 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained

[yuzefovich]

TFTR!

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[andreimatei]

if you know anything about the failure, say on #43949

[yuzefovich]

An unrelated flake.

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)