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

bindinfo: fix bugs when using bindings #14263

Merged
merged 4 commits into from Feb 4, 2020
Merged

Conversation

alivxxx
Copy link
Contributor

@alivxxx alivxxx commented Dec 27, 2019

What problem does this PR solve?

  • It is possible to create a binding that contains columns which do not exist in table.
  • When the binding contains operators like >= 10, but latter the query was >= -1, tidb will panic when binding hints.
  • When we drop session binding and the binding only exists in global level, it is still able to use that global binding in session.

What is changed and how it works?

  • Add check before we create bindings.
  • Use the binding process like master, which is both robust and cleaner.
  • When we found a session binding, no matter the status, it will not look into the global bindings again.

Check List

Tests

  • Unit test
    Code changes

  • Has exported function/method change

Side effects

  • None

Related changes

  • None

Release note

  • Fix bugs when using bindings to bind sql.

@alivxxx alivxxx added the type/bug-fix This PR fixes a bug. label Dec 27, 2019
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

bindinfo/cache.go Show resolved Hide resolved
func (e *SQLBindExec) createSQLBind(ctx context.Context) error {
sqlExec := e.ctx.(sqlexec.SQLExecutor)
// Use explain to check the validity of bind sql.
recordSets, err := sqlExec.Execute(ctx, fmt.Sprintf("explain %s", e.bindSQL))
Copy link
Contributor

Choose a reason for hiding this comment

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

explain is much more expensive than parsing and plan building?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but it much more easier by using explain, and it could also check the privilege of select tables.

Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

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

LGTM

@eurekaka
Copy link
Contributor

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 15, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jan 15, 2020

Your auto merge job has been accepted, waiting for 14470

@sre-bot
Copy link
Contributor

sre-bot commented Jan 15, 2020

/run-all-tests

@eurekaka
Copy link
Contributor

eurekaka commented Feb 3, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Feb 3, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Feb 3, 2020

@lamxTyler merge failed.

@eurekaka
Copy link
Contributor

eurekaka commented Feb 3, 2020

/run-unit-test

@alivxxx
Copy link
Contributor Author

alivxxx commented Feb 3, 2020

/run-all-tests

@eurekaka eurekaka merged commit 1cc43bb into pingcap:release-3.0 Feb 4, 2020
@alivxxx alivxxx deleted the bind-3.0 branch February 6, 2020 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/can-merge Indicates a PR has been approved by a committer. type/bug-fix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants