distsqlrun: make windower respect the memory limits

[yuzefovich]

distsqlrun: make windower respect the memory limits

Previously, the windower didn't respect the memory limit testing knob and
the setting. Now this is fixed. There is one caveat though: windower
requires some amount of RAM to store its intermediate results, so if the
testing knob is lower than that, the limit is overwritten, but if the
cluster setting is insufficient, an error is returned.

Fixes: #40294.

distsqlrun: make router output respect memory limit setting

Release note: None

[cockroach-teamcity]

This change is 

[jordanlewis]

Generally but would like to get alfonso's review upon his return.

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

---

pkg/sql/distsqlrun/server.go, line 102 at r1 (raw file):

	"set to true to enable use of disk for distributed sql window functions",
	true,
)

I think that adding this variable is unnecessary. I don't see when it would be useful to turn it off, unless we use it in tests. I know we have the other ones, but I think we should delete them soon.

---

pkg/sql/distsqlrun/windower.go, line 278 at r1 (raw file):

		allRowsPartitioned := rowcontainer.MakeHashMemRowContainer(rc)
		w.allRowsPartitioned = &allRowsPartitioned
		if err := w.allRowsPartitioned.Init(

This seems to be identical in both branches of the if - is there some DRY to do here?

[yuzefovich]

Thanks for the review!

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

---

pkg/sql/distsqlrun/server.go, line 102 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

I think that adding this variable is unnecessary. I don't see when it would be useful to turn it off, unless we use it in tests. I know we have the other ones, but I think we should delete them soon.

Yeah, I wasn't sure whether the setting was useful or not and decided just to follow the pattern of joins and sorts.

---

pkg/sql/distsqlrun/windower.go, line 278 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

This seems to be identical in both branches of the if - is there some DRY to do here?

There is a subtle difference - in the upper we create a disk backed container, and in the bottom one only an in-memory container.

[asubiotto]

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

---

pkg/sql/distsqlrun/server.go, line 102 at r1 (raw file):

Previously, yuzefovich wrote…

Yeah, I wasn't sure whether the setting was useful or not and decided just to follow the pattern of joins and sorts.

+1 I think we should remove the join and sort knobs too. I think wanting to exit rather than spilling to disk is a legit use case (maybe?) but that can be achieved by reducing the disk monitor limit (i.e. "oops I ran out of disk space"). This would also simplify the code.

---

pkg/sql/distsqlrun/windower.go, line 108 at r1 (raw file):

// memRequiredByWindower indicates the minimum amount of RAM (in bytes) that
// the windower needs.
const memRequiredByWindower = 100000

Maybe easier to read with a multiplication (e.g. 100 * 1024)

---

pkg/sql/distsqlrun/windower.go, line 278 at r1 (raw file):

Previously, yuzefovich wrote…

There is a subtle difference - in the upper we create a disk backed container, and in the bottom one only an in-memory container.

Can we remove this branch if we remove the cluster setting? The testing knob should enforce a hard limit so the only difference would be the monitor that the hash disk backed row container is created with.

---

pkg/sql/rowcontainer/hash_row_container.go, line 1011 at r1 (raw file):

	RowIterator

	container HashRowContainer

Why is this changing to a HashRowContainer? I feel like intuitively we should only ever use spillable row containers. This seems like an artifact of the cluster setting's ability to turn off any disk spilling.

[yuzefovich]

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

---

pkg/sql/distsqlrun/server.go, line 102 at r1 (raw file):

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

+1 I think we should remove the join and sort knobs too. I think wanting to exit rather than spilling to disk is a legit use case (maybe?) but that can be achieved by reducing the disk monitor limit (i.e. "oops I ran out of disk space"). This would also simplify the code.

I agree. I removed the newly introduced setting and will remove others in the follow-up PR.

---

pkg/sql/distsqlrun/windower.go, line 108 at r1 (raw file):

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

Maybe easier to read with a multiplication (e.g. 100 * 1024)

Done.

---

pkg/sql/distsqlrun/windower.go, line 278 at r1 (raw file):

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

Can we remove this branch if we remove the cluster setting? The testing knob should enforce a hard limit so the only difference would be the monitor that the hash disk backed row container is created with.

Refactored - removing the setting allowed for removing that branch.

---

pkg/sql/rowcontainer/hash_row_container.go, line 1011 at r1 (raw file):

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

Why is this changing to a HashRowContainer? I feel like intuitively we should only ever use spillable row containers. This seems like an artifact of the cluster setting's ability to turn off any disk spilling.

Exactly, that was only needed to support the setting. Removed.

[asubiotto]

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

---

pkg/sql/distsqlrun/windower.go, line 209 at r2 (raw file):

		processorID,
		output,
		nil, /* memMonitor */

Should this be nil rather than initialized after the monitor is determined? Might deserve a comment

[yuzefovich]

TFTRs!

bors r+

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

---

pkg/sql/distsqlrun/windower.go, line 209 at r2 (raw file):

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

Should this be nil rather than initialized after the monitor is determined? Might deserve a comment

Good eye - I think I left as nil as a TODO but forgot to leave the TODO comment and actually refactor the code. Done.

[craig]

Build succeeded

• GitHub CI (Cockroach)