release-24.1.0-rc: sql: fix crash when planning stats collection on virtual col with UDT #124064
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
Thanks for opening a backport. Please check the backport criteria before merging:
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
Also, please add a brief release justification to the body of your PR to justify this |
blathers-crl
bot
added
the
backport
|
This change is |
yuzefovich
approved these changes
May 13, 2024
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @mgartner)
mgartner
approved these changes
May 14, 2024
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status:
mgartner
approved these changes
May 14, 2024
There was a problem hiding this comment.
Reviewable status:
rafiss
approved these changes
May 14, 2024
| if err := txn.KV().SetFixedTimestamp(ctx, *details.AsOf); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| planCtx := dsp.NewPlanningCtx(ctx, evalCtx, nil /* planner */, txn.KV(), FullDistribution) | ||
| dsp := innerP.DistSQLPlanner() | ||
| planCtx := dsp.NewPlanningCtx(ctx, innerEvalCtx, innerP, txn.KV(), FullDistribution) |
There was a problem hiding this comment.
i would have expected that we could pass in jobsPlanner here (rather than innerP or nil), but looks like what you have here works.
There was a problem hiding this comment.
It has to be innerP in order for the SemaContext to pick up the correct planner (with the txn) here.
CREATE STATISTICS executes in multiple layers: the "outer" layer is a normal execution of the CREATE STATISTICS statement. During this execution, we create, start, and await a CreateStats job. The CreateStats job in turn starts an "inner" layer which uses a separate internal transaction to plan and execute distributed statistics collection. Within the inner layer, we were using the job's planner (JobExecContext) to plan distributed stats collection. This planner has no associated transaction. If it weren't for user-defined types, this would be fine, but user-defined types must be resolved using a transaction. We had a hack in place to set the evalCtx.Txn to the internal transaction in order to execute collection on normal columns with user-defined type. But this hack did not work for virtual computed columns with user-defined type, because type-checking their expressions uses more facilites of the planner than just the evalCtx. (Specifically the schemaResolver.) So, instead of setting the evalCtx.Txn, we create a new "inner" planner that is associated with the internal transaction of the inner layer. This works for all columns with user-defined type. Fixes: cockroachdb#123821 Release note (bug fix): Fix a crash introduced in v24.1.0-beta.2 that could occur when planning statistics collection on a table with a virtual computed column using a user-defined type when the newly-introduced cluster setting `sql.stats.virtual_computed_columns.enabled` is set to true. (The setting was introduced in v24.1.0-alpha.1 default true.)
michae2
force-pushed
the
backport24.1.0-rc-123926
branch
from
ab7b7e3
to
08e6bdb
Compare
|
Trying a no-op amend to see if that will kick off CI... |
|
I think the branch protection rules may need updating to allow the merge button to be used. |
celiala
merged commit a145a8f
into
cockroachdb:release-24.1.0-rc
19 of 20 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport 1/1 commits from #123926.
/cc @cockroachdb/release
CREATE STATISTICS executes in multiple layers: the "outer" layer is a normal execution of the CREATE STATISTICS statement. During this execution, we create, start, and await a CreateStats job. The CreateStats job in turn starts an "inner" layer which uses a separate internal transaction to plan and execute distributed statistics collection.
Within the inner layer, we were using the job's planner (JobExecContext) to plan distributed stats collection. This planner has no associated transaction. If it weren't for user-defined types, this would be fine, but user-defined types must be resolved using a transaction.
We had a hack in place to set the evalCtx.Txn to the internal transaction in order to execute collection on normal columns with user-defined type. But this hack did not work for virtual computed columns with user-defined type, because type-checking their expressions uses more facilites of the planner than just the evalCtx. (Specifically the schemaResolver.)
So, instead of setting the evalCtx.Txn, we create a new "inner" planner that is associated with the internal transaction of the inner layer. This works for all columns with user-defined type.
Fixes: #123821
Release note (bug fix): Fix a crash introduced in v24.1.0-beta.2 that could occur when planning statistics collection on a table with a virtual computed column using a user-defined type when the newly-introduced cluster setting
sql.stats.virtual_computed_columns.enabledis set to true. (The setting was introduced in v24.1.0-alpha.1 default true.)Release justification: fix for GA-blocker node crash.