release-23.1.12-rc: sql: fix usage of streamer with multiple column families

[yuzefovich]

Backport 2/2 commits from #113019.

/cc @cockroachdb/release

---

Previously, we had the following bug in how the streamer was used under the following conditions:

• we're looking up into a table with at least 3 column families
• not all column families are needed, so we end up creating "family-specific" spans (either Gets or Scans - the latter is possible when multiple contiguous families are needed)
• the streamer is used with OutOfOrder mode (which is the case for index joins when MaintainOrdering is false and for lookup joins when MaintainLookupOrdering is false)
• the index we're looking up into is split across multiple ranges.

In such a scenario we could end up with KVs from different SQL rows being intertwined because the streamer could execute a separate BatchRequest for those rows, and in case TargetBytes limit estimate was insufficient, we'd end up creating "resume" batches, at which point the "results" stream would be incorrect. Later, at the SQL fetcher level this could either manifest as an internal "non-nullable column with no value" error or with silent incorrect output (we'd create multiple output SQL rows from a true single one).

This problem is now fixed by asking the streamer to maintain the ordering of the requests whenever we have SplitFamilyIDs with more than one family, meaning that we might end up creating family-specific spans.

Note that the non-streamer code path doesn't suffer from this problem because there we only parallelize BatchRequests when neither TargetBytes nor MaxSpanRequestKeys limits are set, which is the requirement for having "resume" batches.

Addresses: #113013.
Epic: None

Release note (bug fix): CockroachDB previously could incorrectly evaluate lookup and index joins into tables with at least 3 column families in some cases (either "non-nullable column with no value" internal error would occur or the query would return incorrect results), and this is now fixed. The bug was introduced in 22.2.

Release justification: bug fix.

[blathers-crl]

Thanks for opening a backport.

Please check the backport criteria before merging:

• Patches should only be created for serious issues or test-only changes.
• Patches should not break backwards-compatibility.
• Patches should change as little code as possible.
• Patches should not change on-disk formats or node communication protocols.
• Patches should not add new functionality.
• Patches must not add, edit, or otherwise modify cluster versions; or add version gates.

If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.

• There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
• The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
• New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
• The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

• What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
• Will this work in a cluster of mixed patch versions? Did we test that?
• If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

[cockroach-teamcity]

This change is 

[yuzefovich]

Are all of the changes protected via a flag or option, new syntax, an environment variable or default-disabled cluster setting?

Not really, but it's a correctness issue that we need to fix unconditionally. We are introducing an additional session variable that could help us with similar bugs in the future.

What are the risks to backporting this change? Can the risks of backporting be mitigated?

InOrder mode of streamer is less efficient, so this could cause performance regression. Choosing InOrder mode when not strictly necessary will never lead to a correctness issue, and should the performance become a concern we can always recommend disabling the streamer to a user.

What are the risks to not backporting?

Our users will continue running into internal errors or incorrect results.

Does this change really need to be backported? Or can it reasonably wait until the next major release to be addressed?

Yes, it fixes a critical correctness issue.

[mgartner]

LGTM