[usage] Batch lookup Workspaces to fix too many placeholders error

[easyCZ]

Description

Ensures that we do not include more than 2^16 placeholders in a WHERE field IN (values) prepared query.
https://bugs.mysql.com/bug.php?id=101630

Related Issue(s)

Fixes #10642

How to test

Unit tests

Release Notes

NONE

Documentation

NONE

[jankeromnes]

Ensures that we do not include more than 2^16 placeholders in a WHERE field IN (values) prepared query.
https://bugs.mysql.com/bug.php?id=101630

🤯 is that limit really 2^16? I assumed it would be something like 1000. Anyway, I guess the unit tests will tell -- will take a look today. 👀

[easyCZ]

Ensures that we do not include more than 2^16 placeholders in a WHERE field IN (values) prepared query.
bugs.mysql.com/bug.php?id=101630

🤯 is that limit really 2^16? I assumed it would be something like 1000. Anyway, I guess the unit tests will tell -- will take a look today. 👀

Yeah I was also surprised.

[jankeromnes]

Won't this always process one more batch than necessary?

(math.Ceil already rounds up the number of necessary batches, then you go from 0 to batches both included?)

Suggested change

	batches := int(math.Ceil(float64(items / listBatchSize)))
	for i := 0; i <= batches; i++ {
	batches := int(math.Ceil(float64(items / listBatchSize)))
	for i := 0; i < batches; i++ {

[easyCZ]

I've simplified this a bit. The issue was with float64(items / listBatchSize) which actually performed integer division (div). What it needs to be is float64(items) / listBatchSize because the numerator determines the resulting type of the division operation, here it's floating point division. Which then works with Ceil to always run at least one batch.

[jankeromnes]

For my own curiosity: In Go, list[lower:upper] is already safe against out-of-bounds, right? E.g. list[0:10] on a list with 4 items would return a list with 4 items without errors?

[easyCZ]

In go, the upper bound from a slice is not safe for out of bounds, it panics https://goplay.tools/snippet/SgSiwcAPfXC

[jankeromnes]

Code looks good and seems to work (according to the unit test).

I find it hard to trust code just based on unit tests (e.g. production workload might be significantly different than what the unit tests do, as we've seen for this bug), but in this case, it's okay to test in production since this component is not on the critical path yet. 😊

Adding a hold because of optional questions/remarks:

/hold

[easyCZ]

I find it hard to trust code just based on unit tests (e.g. production workload might be significantly different than what the unit tests do, as we've seen for this bug)

How do you propose we solve this problem?

In general there are a couple of constraints:

1. There's only one production, staging helps (but questionably it's planned to be removed) but still doesn't come close to production
2. Engineers shouldn't really have direct DB access to production. This should be used in emergency situations but not as part of a development cycle, it is a main source of compromised systems - attack on an employee.
3. If CD exists, and so does a fast rollback, then it should be encouraged to "test in production" - given there are sufficient safe-guards to prevent the system going crazy - feature flags or read only mode are good examples

[easyCZ]

/unhold