sql: do not re-run optbuild before collecting index recommendations#117433
sql: do not re-run optbuild before collecting index recommendations#117433craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
|
This is broken, hold off for a bit... |
|
Ok, should be RFAL now. |
DrewKimball
left a comment
There was a problem hiding this comment.
Great job figuring this out! I hope we can get rid of some of these pesky memo references soon.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @michae2 and @yuzefovich)
pkg/sql/instrumentation.go line 1048 at r1 (raw file):
opc := &planner.optPlanningCtx f := opc.optimizer.Factory() if f.Memo().IsEmpty() {
I think this should probably include a nil check - ReleaseMemo leaves the memo nil, and is called in buildExecMemo.
yuzefovich
left a comment
There was a problem hiding this comment.
Nice job tracking this down! Looks good to me, but I defer to Drew's approval.
Reviewed 3 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status:
-- commits line 7 at r2:
nit: s/detatching/detaching/ and s/detatched/detached/ below.
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.
|
TFTRs! bors r=DrewKimball |
michae2
left a comment
There was a problem hiding this comment.
Reviewable status:
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit:
s/detatching/detaching/ands/detatched/detached/below.
Oh! Good Catch.
pkg/sql/instrumentation.go line 1048 at r1 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
I think this should probably include a nil check -
ReleaseMemoleaves the memo nil, and is called inbuildExecMemo.
Good call! Thank you!
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error setting reviewers, but backport branch blathers/backport-release-23.2-117433 is ready: POST https://api.github.com/repos/cockroachdb/cockroach/pulls/117453/requested_reviewers: 422 Reviews may only be requested from collaborators. One or more of the teams you specified is not a collaborator of the cockroachdb/cockroach repository. [] Backport to branch 23.2.x failed. See errors above. error setting reviewers, but backport branch blathers/backport-release-23.2.0-rc-117433 is ready: POST https://api.github.com/repos/cockroachdb/cockroach/pulls/117454/requested_reviewers: 422 Reviews may only be requested from collaborators. One or more of the teams you specified is not a collaborator of the cockroachdb/cockroach repository. [] Backport to branch 23.2.0-rc failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
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.