release-23.2.0-rc: sql: do not re-run optbuild before collecting index recommendations

[blathers-crl]

Backport 1/1 commits from #117433 on behalf of @michae2.

/cc @cockroachdb/release

---

In #85343 for 22.2 we added automatic collection of index recommendations for DML statements. This collection occurred after execution, and initially re-ran optbuild for the query, before doing the usual index recommendation generation steps of (1) detaching the memo, (2) optimizing with hypothetical indexes, and then (3) restoring the detached memo.

In #99081 for 23.2 we moved collection of index recommendations from after execution to between planning and execution, which simplified some things. But this revealed that some queries refer to the memo (and its metadata) during execution, and re-running optbuild can change the contents of the memo (and its metadata) from how they were after planning, especially if the original memo was cached and re-used.

I think this initial optbuild step was added to ensure we always have a root expression in the memo. This commit changes index recommendation collection to only run optbuild if the memo is empty, and otherwise use the memo that comes out of planning.

Fixes: #117307

Release note (bug fix): Fix a bug introduced in 23.2 that causes internal errors and panics when certain queries run with automatic index recommendation collection enabled.

---

Release justification: fix for GA blocker.

[blathers-crl]

Thanks for opening a backport.

Please check the backport criteria before merging:

• Backports should only be created for serious
  issues or test-only changes.
• Backports should not break backwards-compatibility.
• Backports should change as little code as possible.
• Backports should not change on-disk formats or node communication protocols.
• Backports should not add new functionality (except as defined
  here).
• Backports must not add, edit, or otherwise modify cluster versions; or add version gates.
• All backports must be reviewed by the owning areas TL and one additional
  TL. For more information as to how that review should be conducted, please consult the backport
  policy.

If your backport adds new functionality, please ensure that the following additional criteria are satisfied:

• 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. State changes must be further protected such that nodes running old binaries will not be negatively impacted by the new state (with a mixed version test added).
• The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.
• Your backport must be accompanied by a post to the appropriate Slack
  channel (#db-backports-point-releases or #db-backports-XX-X-release) for awareness and discussion.

Also, please add a brief release justification to the body of your PR to justify this
backport.

[cockroach-teamcity]

This change is 

[michae2]

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

The changes are all behind existing cluster setting sql.metrics.statement_details.index_recommendation_collection.enabled, but this setting is default-enabled.

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

Risk is that automatic index recommendation collection would not work correctly for some statements.

What are the risks to not backporting?

Some prepared statements can panic the gateway note when running with automatic index recommendation collection.

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

This panic is new in 23.2 and might affect multiple kinds of queries. (Any query which uses the planning memo at execution time. So far we've only found one but there could be others.)

[mgartner]

Great job tracking this tricky bug down!

This part of the logic around index recommendations is very hard to understand. That's likely a major reason the bug was caused in the first place—in SetIndexRecommendations it's unclear what the state of opc.optimizer.factory could be, and it's not obvious that rebuilding the memo could mess with the metadata. How can we untangle this code and make it hard or impossible to violate the invariants we've discovered with this bug?

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @blathers-crl[bot] and @rafiss)

---

pkg/sql/instrumentation.go line 1050 at r1 (raw file):

			if f.Memo() == nil || f.Memo().IsEmpty() {
				// Run optbuild to create a memo with a root expression, if the current
				// memo is empty.

nit: In a follow-up PR explain when and why the factory's memo is empty here. It's when a cached memo is reused, correct?