sql: set index recommendations after planning but before execution #99081

yuzefovich · 2023-03-20T23:46:57Z

This commit moves the call to set the index recommendations to be done
right after planning was completed. Previously, it was done after the
execution, but it makes more sense to do it after planning. This also
allows us to remove the check on the txn still being open.

This required clarifying how instrumentationHelper.indexRecs is used.
Previously, it was used for two purposes:

for recording recommendations to be included in the
statement_statistics system table
for showing when executing EXPLAIN statement.

These two usages have somewhat different requirements, so this commit
splits them out into two different slices. This also allows us to reuse
the recommendations from the latter should we choose to generate the
recommendations for the former (previously, this would result in
redundant regeneration of the recommendations).

Epic: None

Release note: None

blathers-crl · 2023-03-20T23:47:01Z

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

cockroach-teamcity · 2023-03-20T23:47:06Z

This change is

yuzefovich · 2023-03-20T23:47:12Z

cc @ZhouXing19

ZhouXing19 · 2023-03-21T19:22:02Z

Seems that these changes made TestExecBuild_explain failed, as now there are no index recommendations: 1 and CREATE INDEX messages for EXPLAIN (TYPES) SELECT * FROM t WHERE v > 123.

yuzefovich · 2023-03-21T21:40:04Z

This required some more adjustments than I initially made, should be fixed now.

michae2

Reviewed 2 of 4 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @yuzefovich)

pkg/sql/instrumentation.go line 795 at r2 (raw file):

		// If the statement is an EXPLAIN, then we might have already generated
		// the index recommendations. If so, we can skip generation here.
		if len(ih.explainIndexRecs) > 0 {

Should this check for ih.explainIndexRecs != nil instead of len > 0? (In other words, is it possible that EXPLAIN will have zero index recommendations?)

mgartner · 2023-03-22T21:35:06Z

pkg/sql/instrumentation.go line 795 at r2 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Should this check for ih.explainIndexRecs != nil instead of len > 0? (In other words, is it possible that EXPLAIN will have zero index recommendations?)

Ya I think a nil check would prevent duplicate work in the case there are no recommendations.

This commit moves the call to set the index recommendations to be done right after planning was completed. Previously, it was done after the execution, but it makes more sense to do it after planning. This also allows us to remove the check on the txn still being open. This required clarifying how `instrumentationHelper.indexRecs` is used. Previously, it was used for two purposes: - for recording recommendations to be included in the `statement_statistics` system table - for showing when executing EXPLAIN statement. These two usages have somewhat different requirements, so this commit splits them out into two different slices. This also allows us to reuse the recommendations from the latter should we choose to generate the recommendations for the former (previously, this would result in redundant regeneration of the recommendations). Epic: None Release note: None

yuzefovich

TFTRs!

bors r+

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

craig · 2023-03-23T03:28:31Z

Build succeeded:

Bazel Essential CI (Cockroach)

knz · 2023-04-02T11:58:37Z

I think this should be backported?

yuzefovich · 2023-04-03T16:18:25Z

Hm, I don't think this needs to be backported - it's just a nice-to-have cleanup IMO. @mgartner WDYT?

In cockroachdb#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) detatching the memo, (2) optimizing with hypothetical indexes, and then (3) restoring the detatched memo. In cockroachdb#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, expecially 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: cockroachdb#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.

In cockroachdb#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 cockroachdb#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: cockroachdb#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.

117433: sql: do not re-run optbuild before collecting index recommendations r=DrewKimball a=michae2 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. Co-authored-by: Michael Erickson <[email protected]>

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.

yuzefovich requested review from mgartner and a team March 20, 2023 23:46

yuzefovich force-pushed the move-index-recs branch from 1f299e2 to d061da4 Compare March 21, 2023 21:37

michae2 approved these changes Mar 22, 2023

View reviewed changes

yuzefovich force-pushed the move-index-recs branch from d061da4 to 0cbb425 Compare March 23, 2023 02:26

yuzefovich commented Mar 23, 2023

View reviewed changes

craig bot merged commit 436613f into cockroachdb:master Mar 23, 2023

yuzefovich deleted the move-index-recs branch March 23, 2023 03:33

michae2 mentioned this pull request Jan 5, 2024

execbuilder: index out of range panic in mkFKCheckErr #117307

Closed

michae2 mentioned this pull request Jan 5, 2024

sql: do not re-run optbuild before collecting index recommendations #117433

Merged

This was referenced Jan 6, 2024

release-23.2: sql: do not re-run optbuild before collecting index recommendations #117453

Merged

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: set index recommendations after planning but before execution #99081

sql: set index recommendations after planning but before execution #99081

yuzefovich commented Mar 20, 2023 •

edited

Loading

blathers-crl bot commented Mar 20, 2023

cockroach-teamcity commented Mar 20, 2023

yuzefovich commented Mar 20, 2023

ZhouXing19 commented Mar 21, 2023

yuzefovich commented Mar 21, 2023 •

edited

Loading

michae2 left a comment

mgartner commented Mar 22, 2023

yuzefovich left a comment

craig bot commented Mar 23, 2023

knz commented Apr 2, 2023

yuzefovich commented Apr 3, 2023

sql: set index recommendations after planning but before execution #99081

sql: set index recommendations after planning but before execution #99081

Conversation

yuzefovich commented Mar 20, 2023 • edited Loading

blathers-crl bot commented Mar 20, 2023

cockroach-teamcity commented Mar 20, 2023

yuzefovich commented Mar 20, 2023

ZhouXing19 commented Mar 21, 2023

yuzefovich commented Mar 21, 2023 • edited Loading

michae2 left a comment

Choose a reason for hiding this comment

mgartner commented Mar 22, 2023

yuzefovich left a comment

Choose a reason for hiding this comment

craig bot commented Mar 23, 2023

knz commented Apr 2, 2023

yuzefovich commented Apr 3, 2023

yuzefovich commented Mar 20, 2023 •

edited

Loading

yuzefovich commented Mar 21, 2023 •

edited

Loading