Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

expression: fix unexpected panic when doing isNullRejected check (#22173) #22327

Merged
merged 4 commits into from
Jan 14, 2021

Conversation

ti-srebot
Copy link
Contributor

@ti-srebot ti-srebot commented Jan 11, 2021

cherry-pick #22173 to release-4.0
You can switch your code base to this Pull Request by using git-extras:

# In tidb repo:
git pr 22327

After apply modifications, you can push your change to this PR via:

git push [email protected]:ti-srebot/tidb.git pr/22327:ti-srebot:release-4.0-cdcb0ffa3412

What problem does this PR solve?

Issue Number: close #22098

Problem Summary:
TiDB server panics when executing a prepared SQL with prepared-plan-cache enabled.

What is changed and how it works?

What's Changed:
When creating Constant in foldConstant for NullRejectCheck case, we set its DeferredExpr to nil.

How it Works:

  1. The panic is caused by reevaluating DeferredExpr of the constant created in foldConstant. When creating the constant in foldConstant, we evaluate a dummyScalarFunc instead of the original ScalarFunction to get the constant value while we set DeferredExpr to the original ScalarFunction. When the constant is used as argument for another function, it is reevaluated by evaluating the original ScalarFunction, which cannot be done in the context and leads to panic.
  2. The constant mentioned above is created to compose the result expression of EvaluateExprWithNull when InNullRejectCheck = true. We just check whether the result expression is null or false and then let it die. Basically, the constant is used once briefly and will not be retained for a long time. Hence setting DeferredExpr of the constant to nil is ok.

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test

Release note

  • fix unexpected panic when executing certain prepared SQL with prepared-plan-cache enabled.

@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot
Copy link
Contributor Author

@xuyifangreeneyes you're already a collaborator in bot's repo.

@xuyifangreeneyes
Copy link
Contributor

/run-all-tests

@xuyifangreeneyes
Copy link
Contributor

/run-all-tests

@xuyifangreeneyes
Copy link
Contributor

/run-all-tests

Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 14, 2021
Copy link
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-srebot ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 14, 2021
@qw4990 qw4990 merged commit efa487e into pingcap:release-4.0 Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression sig/planner SIG: Planner status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug. type/4.0-cherry-pick
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants