distsqlrun: pool TableReaders

[jordanlewis]

And permit ProcOutputHelper and ProcessorBase to be reset without
throwing away all slice memory.

Extracted from #30556.

Release note: None

[cockroach-teamcity]

This change is 

[jordanlewis]

name                          old time/op    new time/op    delta
FlowSetup/distribute=true-8     44.2µs ± 3%    44.4µs ± 2%    ~     (p=0.563 n=10+9)
FlowSetup/distribute=false-8    34.5µs ± 5%    32.6µs ± 2%  -5.42%  (p=0.000 n=10+9)

name                          old alloc/op   new alloc/op   delta
FlowSetup/distribute=true-8     38.6kB ± 0%    36.4kB ± 0%  -5.60%  (p=0.000 n=9+9)
FlowSetup/distribute=false-8    30.2kB ± 0%    28.0kB ± 0%  -7.22%  (p=0.000 n=9+9)

name                          old allocs/op  new allocs/op  delta
FlowSetup/distribute=true-8        344 ± 0%       341 ± 0%  -0.84%  (p=0.000 n=10+10)
FlowSetup/distribute=false-8       258 ± 0%       255 ± 0%  -1.16%  (p=0.000 n=10+10)

[jordanlewis]

This is kind of weird - I can't reproduce the CI failure locally.

[RaduBerinde]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/distsqlrun/processors.go, line 163 at r1 (raw file):

		}
	} else {
		h.outputTypes = types

I think this might be what the crash is about. Here we're using the given types directly.

[nit] also add a comment in this block // No projection or renders., it's hard to see the high level logic

[RaduBerinde]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/distsqlrun/processors.go, line 66 at r1 (raw file):

	renderExprs []exprHelper
	// outputCols is set if we have a projection. Only one of renderExprs and
	// outputCols can have length > 0.

[nit] 0-length projections are possible, which would be an empty non-nil outputCols. Can you add that information in a comment here? And say is not nil instead of length > 0 in these comments

[jordanlewis]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/distsqlrun/processors.go, line 66 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] 0-length projections are possible, which would be an empty non-nil outputCols. Can you add that information in a comment here? And say is not nil instead of length > 0 in these comments

Hmm, I didn't realize that. That makes getting this code right harder. I think we'll need to store a boolean then, because now there's no way to distinguish empty renderExprs caused by the pool trying to retain slice memory, vs empty renderExprs caused by a 0 length projection.

---

pkg/sql/distsqlrun/processors.go, line 163 at r1 (raw file):

Previously, RaduBerinde wrote…

I think this might be what the crash is about. Here we're using the given types directly.

[nit] also add a comment in this block // No projection or renders., it's hard to see the high level logic

done.

[jordanlewis]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/sql/distsqlrun/processors.go, line 66 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Hmm, I didn't realize that. That makes getting this code right harder. I think we'll need to store a boolean then, because now there's no way to distinguish empty renderExprs caused by the pool trying to retain slice memory, vs empty renderExprs caused by a 0 length projection.

Oh, wait, I see, this only matters for outputCols, which we aren't trying to save the memory of.

[RaduBerinde]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/sql/distsqlrun/processors.go, line 66 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Oh, wait, I see, this only matters for outputCols, which we aren't trying to save the memory of.

Can you explicitly mention that 0-length projections are possible, in which case outputCols has length 0 but is not nil.

[jordanlewis]

Done. TFTR!

[jordanlewis]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)