colexec: miscellaneous cleanup

[yuzefovich]

colexec: clean up external distinct test a bit

Release note: None

colbuilder: enforce unique memory monitor names

Our disk-spilling infrastructure (namely, diskSpillerBase) uses the
name of the limited memory monitor of its in-memory operator in order to
distinguish the "memory budget exceeded" errors between the ones it
needs to catch and the ones it needs to propagate further. In
particular, previously it was possible that the in-memory chains within
two disk-backed sorters created for the external distinct (one in the
fallback strategy and another when the output ordering is needed, on top
of the external distinct) would have the same memory monitor names. This
is now fixed by appending a unique suffix to all names and is enforced
via an invariants assertion in the test setup.

Note that even with the previous behavior no actual bug would occur
because the monitors with the same name were on "different levels of the
tree" and there was the correct catcher in-between. So this commit
simply restores the assumption that we had implicitly without fixing any
production bugs.

Release note: None

[cockroach-teamcity]

This change is 

[mgartner]

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

---

pkg/sql/colexec/colbuilder/execplan.go, line 1537 at r2 (raw file):

	ctx context.Context, flowCtx *execinfra.FlowCtx, name string,
) (*mon.BoundAccount, string) {
	monitorName := r.getMemMonitorName(name + "-limited")

nit: it's a bit confusing that the name (which appears to be the monitor name) is an argument, but a modified monitor name is also returned. Not sure if it'd necessarily be better, but you could change the name string argument here and in the function below to opName string, pid execinfrapb.ProcessorID, and pass both and a suffix string to a getMemMonitorName(opName string, pid execinfrapb.ProcessorID, suffix string). (Not sure opName is the right term, maybe there's something better). This would eliminate some duplicat fmt.Sprintf's I think.

[yuzefovich]

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

---

pkg/sql/colexec/colbuilder/execplan.go, line 1537 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: it's a bit confusing that the name (which appears to be the monitor name) is an argument, but a modified monitor name is also returned. Not sure if it'd necessarily be better, but you could change the name string argument here and in the function below to opName string, pid execinfrapb.ProcessorID, and pass both and a suffix string to a getMemMonitorName(opName string, pid execinfrapb.ProcessorID, suffix string). (Not sure opName is the right term, maybe there's something better). This would eliminate some duplicat fmt.Sprintf's I think.

I like this idea, done. Overall, this mechanism of monitor name matching is quite brittle, and the code is not very pretty, but at least it does the job :)

[yuzefovich]

TFTR!

bors r+

[michae2]

Reviewed 1 of 2 files at r1, 3 of 4 files at r2, 1 of 1 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner and @yuzefovich)

---

pkg/sql/colexec/colbuilder/execplan.go, line 1537 at r2 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

I like this idea, done. Overall, this mechanism of monitor name matching is quite brittle, and the code is not very pretty, but at least it does the job :)

Nit: Now that you've changed it, I don't think you need the hyphen on "-limited" any more.

[yuzefovich]

bors r-

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

---

pkg/sql/colexec/colbuilder/execplan.go, line 1537 at r2 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Nit: Now that you've changed it, I don't think you need the hyphen on "-limited" any more.

Indeed, nice catch!

[craig]

Canceled.

[yuzefovich]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)