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 the panic when we use unix_time expression in prepare and execute statement. #17855

Merged
merged 12 commits into from
Jun 15, 2020

Conversation

Reminiscent
Copy link
Contributor

@Reminiscent Reminiscent commented Jun 8, 2020

What problem does this PR solve?

Issue Number: close #17727

Problem Summary:
When we set prepare-plan-cache = true and use prepare && execute statement. Some time-related expression such as now() will be wrapped into constant.deferredExpr for delayed calculation. But the expression unix_timestamp is special. When there is no parameter, it should be placed in constant.deferredExpr for delay calculation. If there is a parameter, it should not be put into constant.deferredExpr, otherwise, it will cause access to nil pointer

What is changed and how it works?

Special handling of the case of the expression unix_timestamp for funcCallToExpression.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test

Release note

  • [Bug-fix] Fix the panic when we use unix_time expression in prepare and execute statement.

@Reminiscent Reminiscent changed the title expression: fix the wrong behavior of some Const.DeferredExpr's related functions expression: fix the panic when we use unix_time expression in prepare and execute statement. Jun 9, 2020
@Reminiscent
Copy link
Contributor Author

@lysu PTAL

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.

The current impl is not the same thing as your PR's description.
Why just take the UNIX_TIMESTAMP into special consideration?
@Reminiscent

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

@Reminiscent
Copy link
Contributor Author

@winoros It's my mistake, I have updated the PR description.

@zz-jason zz-jason added status/LGT1 Indicates that a PR has LGTM 1. sig/planner SIG: Planner labels Jun 12, 2020
@codecov
Copy link

codecov bot commented Jun 12, 2020

Codecov Report

Merging #17855 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #17855   +/-   ##
===========================================
  Coverage   79.4785%   79.4785%           
===========================================
  Files           524        524           
  Lines        142168     142168           
===========================================
  Hits         112993     112993           
  Misses        20033      20033           
  Partials       9142       9142           

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added needs-cherry-pick-4.0 status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 15, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jun 15, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 15, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 15, 2020

@Reminiscent merge failed.

@Reminiscent
Copy link
Contributor Author

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Jun 15, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 15, 2020

@Reminiscent merge failed.

@Reminiscent
Copy link
Contributor Author

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Jun 15, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 15, 2020

@Reminiscent merge failed.

@Reminiscent
Copy link
Contributor Author

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Jun 15, 2020

/run-all-tests

@sre-bot sre-bot merged commit 8a33340 into pingcap:master Jun 15, 2020
sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Jun 15, 2020
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot
Copy link
Contributor

sre-bot commented Jun 15, 2020

cherry pick to release-4.0 in PR #18002

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/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

P1-[4.0 bug hunting]-[Plancache]-tidb-dashboard causes tidb-server panic when plancache enabled
5 participants