constraint: perform cancel checking when combining constraints

[msirek]

Previously, the combining of constraint spans when one constraint is a
suffix of the other could take a long time and cause the
statement_timeout session setting to not be honored when each
constraint has hundreds or thousands of spans.

The issue is that constraint.Combine has double nested loops to
consider every combination of one span from one constraint with one span
of the other constraint. The building of possibly millions of spans
may take excessive CPU time and allocate excessive amounts of memory.

The fix is to maintain a counter in constraint.Combine and call the
query cancel check function every 16 iterations. The cancel check
function itself will check for query timeout every 1024 iterations, so
effectively every 16K iterations constraint.Combine will perform
cancel checking and abort the query if the timeout has been reached.

Epic: none
Fixes: #111862

Release note (bug fix): This patch fixes an issue where the optimizer
fails to honor the statement_timeout session setting when generating
constrained index scans for queries with large IN lists or = ANY
predicates on multiple index key columns, which may lead to an out
of memory condition on the node.

[cockroach-teamcity]

This change is 

[yuzefovich]

I only have a few nits. Perhaps @DrewKimball should take a quick look too.

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

---

pkg/sql/opt/constraint/constraint_test.go line 501 at r1 (raw file):

		}
	}
	for i, tc := range testData {

nit: isn't there only a single test case? Do you envision that we'll add more test cases in the future, and that's why we have this for loop?

---

pkg/sql/opt/constraint/constraint_test.go line 531 at r1 (raw file):

			j.Combine(&evalCtx, &l, cancelFunc)
			n.Combine(&evalCtx, &p, cancelFunc)
			// The next Combine call will panic within a second or two.

nit: should we actually time how long the cancellation takes and assert that it's lower than some pre-defined constant?

---

pkg/sql/opt/constraint/constraint_test.go line 535 at r1 (raw file):

			// This Combine call panics right away since the iterations target has
			// already been reached.
			require.Panics(t, func() { j.Combine(&evalCtx, &n, cancelFunc) })

nit: perhaps store iterations before calling Combine and then assert that it only increases by 1?

[DrewKimball]

Nice work!

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @cucaroach and @msirek)

---

pkg/sql/opt/constraint/constraint_test.go line 495 at r1 (raw file):

	iterations := 0
	cancelFunc := func() {

[nit] We should move these into the loop (if we keep it around) in case more test cases are added.

[msirek]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @cucaroach, @DrewKimball, and @yuzefovich)

---

pkg/sql/opt/constraint/constraint_test.go line 495 at r1 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

[nit] We should move these into the loop (if we keep it around) in case more test cases are added.

Done

---

pkg/sql/opt/constraint/constraint_test.go line 501 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: isn't there only a single test case? Do you envision that we'll add more test cases in the future, and that's why we have this for loop?

I'm just following the pattern of other tests here, so it's easy to add more tests if we want to.

---

pkg/sql/opt/constraint/constraint_test.go line 531 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: should we actually time how long the cancellation takes and assert that it's lower than some pre-defined constant?

OK, added checking that each cancellation takes less than one second.

---

pkg/sql/opt/constraint/constraint_test.go line 535 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: perhaps store iterations before calling Combine and then assert that it only increases by 1?

It's not going to increase just by 1. It will increase by some factor of the number of spans in the 2 constraints plus maybe extra iterations on the main outer loop. If for some reason this count starts going up due to a future software change, then earlier calls to Combine will panic before the one where we expect it, and the test will fail (currently if I change the if iterations > 2000 check to if iterations > 2000 the test would fail because we reach more than 1000 iteration because combining a with e).

[msirek]

TFTRs!
bors r=yuzefovich,DrewKimball

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @cucaroach, @DrewKimball, and @yuzefovich)

[msirek]

currently if I change the if iterations > 2000 check to if iterations > 2000...

I meant to say:
currently if I change the if iterations > 2000 check to if iterations > 1000...

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)