colexec: fix simpleProjectOp in some cases

[yuzefovich]

simpleProjectOp doesn't work well together with unordered
synchronizers (and possibly other components that can return
coldata.Batches that point to different regions of memory). The
problem is that simpleProjectOp adds a projectingBatch wrapper on
top its input batch, but if "different internally" batches come in, the
wrapper is not updated/reset. Now this is fixed by tracking different
input batches and having a separate projectingBatch for them.

Fixes: #45686.

Release note (bug fix): Previously, an internal error could occur in
CockroachDB when executing queries via the vectorized engine in queries
that contained unordered synchronizers, and now this has been fixed.

[cockroach-teamcity]

This change is 

[asubiotto]

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

---

pkg/sql/colexec/simple_project.go, line 110 at r1 (raw file):

		// We pass in a copy of d.projection just to be safe.
		projBatch = newProjectionBatch(append([]uint32{}, d.projection...))
		d.batches[batch] = projBatch

Do we have any guarantee that this map will grow only as large as there are nodes in the cluster? I'm worried about potential cases in which batch pointers always differ. Don't know when that would be the case though, since when we buffer batches, we usually copy to an output batch (at least in the sorter), right?

[yuzefovich]

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

---

pkg/sql/colexec/simple_project.go, line 110 at r1 (raw file):

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

Do we have any guarantee that this map will grow only as large as there are nodes in the cluster? I'm worried about potential cases in which batch pointers always differ. Don't know when that would be the case though, since when we buffer batches, we usually copy to an output batch (at least in the sorter), right?

We don't have such guarantee. In theory, if an upstream operator instantiates a new batch on its every Next call, this map can grow unboundedly. In practice though I also can't think of an operator that behaves this way.

[jordanlewis]

Maybe another way to fix this could be during planning. We have full knowledge of both arms of the unordered synchronizer. Could we plan the simple project operators before feeding into the unordered synchronizer? Or does that make no sense.

[yuzefovich]

Possibly my explanation and the example are somewhat misleading. I believe the root of the problem is with simpleProjectOp (and the unordered synchronizers in combination with some projecting operators expose that). Let's talk through it during the standup.

[asubiotto]

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

---

pkg/sql/colexec/simple_project.go, line 102 at r2 (raw file):

		projection:                 projection,
		batches:                    make(map[coldata.Batch]*projectingBatch),
		numBatchesLoggingThreshold: 4,

I would maybe make this larger, say 128. I think 4 could be expected.

[yuzefovich]

TFTR!

bors r+

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

---

pkg/sql/colexec/simple_project.go, line 102 at r2 (raw file):

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

I would maybe make this larger, say 128. I think 4 could be expected.

Done.

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[yuzefovich]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)