colexec: close disk resources in tests

[michae2]

Fixes: #106119

colcontainer: close PDQ during TestPartitionedDiskQueueSimulatedExternal

Close the PartitionedDiskQueue at the end of TestPartitionedDiskQueueSimulatedExternal/HashJoin.

Release note: None

---

colexecutils: close SpillingBuffer during TestSpillingBuffer

Release note: None

---

colexec: make some operators implement colexecop.Closer interface

This commit makes the following adjustments that fix "leaked files" in
the disk-spilling tests:

• orderedDistinct now implements colexecop.Closer (needed for
  TestExternalDistinct)
• orderedAggregator and distinctChainOps now implement
  colexecop.Closer (needed for TestExternalHashAggregator)
• TestCrossJoiner is adjusted to explicitly close all closers when
  non-empty ON expression is used (in this case we have selection and
  projection operators on top of the cross joiner which currently don't
  implement the Closer interface).

Release note: None

[cockroach-teamcity]

This change is 

[yuzefovich]

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @michae2 and @rharding6373)

---

-- commits line 21 at r3:
I'm not sure about this. We have this best-effort notion that closing the head operator should trigger closing operators in the whole tree, and I'd rather investigate which operators don't do that. I'm a bit concerned that this commit could be hiding some problems. Even though at the vectorizedFlow level we track most closers separately, it'd still be nice to improve our best-effort closing of everything by closing the head. Thoughts?

[michae2]

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

---

-- commits line 21 at r3:

Previously, yuzefovich (Yahor Yuzefovich) wrote…

I'm not sure about this. We have this best-effort notion that closing the head operator should trigger closing operators in the whole tree, and I'd rather investigate which operators don't do that. I'm a bit concerned that this commit could be hiding some problems. Even though at the vectorizedFlow level we track most closers separately, it'd still be nice to improve our best-effort closing of everything by closing the head. Thoughts?

SGTM. I will try to figure out exactly which operators are not being closed.

[yuzefovich]

@cockroachdb/storage friends please ignore this PR (I temporarily cherry-picked #106195 to see what CI says).

@michae2 hope you don't mind that I looked into this. I investigated several failures in colexec package, and I think I fixed them all in the last commit. Thus, I don't think we need the third commit that always closes the closers in the test harness (my thinking is that we should think through each test case separately, to see whether any "close-propagation" is missing).

[michae2]

Thanks for picking this up! I'm sorry I let it fester. Last commit

Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)

[yuzefovich]

No worries! The only exposed failure was in kvserver package (filed #110293), so we should be good.

bors r=yuzefovich,michae2

[craig]

👎 Rejected by too few approved reviews

[yuzefovich]

Reviewed 33 of 34 files at r4, 3 of 3 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @michae2)

[yuzefovich]

bors r=yuzefovich,michae2

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

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[yuzefovich]

blathers backport 23.1

[mgartner]

Should we also backport this to 23.2 to fixed #112868?

[yuzefovich]

It was merged before 23.2 was cut, so #112868 is a separate issue.