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

planner,executor,parser: support the plan cache for prepared insert/delete/update statements #8107

Merged
merged 2 commits into from
Nov 5, 2018
Merged

planner,executor,parser: support the plan cache for prepared insert/delete/update statements #8107

merged 2 commits into from
Nov 5, 2018

Conversation

dbjoa
Copy link
Contributor

@dbjoa dbjoa commented Oct 30, 2018

What problem does this PR solve?

Support the plan cache for the prepared insert/delete/update statements

What is changed and how it works?

  • Extend rebuildRange() to support the insert/update/delete plans
  • Allow prepared insert/update/delete statements to be cacheable

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Side effects

  • No

Related changes

  • Need to be included in the release note

@sre-bot
Copy link
Contributor

sre-bot commented Oct 30, 2018

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@dbjoa
Copy link
Contributor Author

dbjoa commented Oct 30, 2018

/run-all-tests

@shenli shenli added the contribution This PR is from a community contributor. label Oct 30, 2018
@shenli
Copy link
Member

shenli commented Oct 30, 2018

@dbjoa Thanks!

@dbjoa
Copy link
Contributor Author

dbjoa commented Oct 30, 2018

/run-common-test

@eurekaka eurekaka added sig/planner SIG: Planner type/enhancement The issue or PR belongs to an enhancement. labels Oct 30, 2018
planner/core/common_plans.go Outdated Show resolved Hide resolved
planner/core/common_plans.go Show resolved Hide resolved
node, ok := n.Table.Accept(v)
if !ok {
return n, false
if n.Table != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please give an example where n.Table is nil in INSERT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, then this code change is unreasonable IMHO. By the way, I learned that since we have moved parser to a separate repository, we need to file a PR in https://github.com/pingcap/parser if we want to make changes, and then make update in tidb repo.

@eurekaka
Copy link
Contributor

The failed common-test can be simplified to this test case:

import java.sql.*;

class TiDBTestConnector {
	public static void main(String args[]){
		try{
				ResultSet rs;
				Class.forName("com.mysql.jdbc.Driver");
				Connection con=DriverManager.getConnection("jdbc:mysql://127.0.0.1:4000/test?useServerPrepStmts=true","root","");
				//createTable("testExecuteLargeBatch", "(id BIGINT AUTO_INCREMENT PRIMARY KEY, n INT)");
				PreparedStatement stmt = con.prepareStatement("INSERT INTO testExecuteLargeBatch (n) VALUES (?)", Statement.RETURN_GENERATED_KEYS);
				stmt.setInt(1, 1);
				stmt.addBatch();
				stmt.setInt(1, 2);
				stmt.addBatch();
				stmt.setInt(1, 3);
				stmt.addBatch();
				stmt.setInt(1, 4);
				stmt.addBatch();
				stmt.addBatch("INSERT INTO testExecuteLargeBatch (n) VALUES (5), (6), (7)");
				stmt.setString(1, "eight");
				stmt.addBatch();
				stmt.addBatch("INSERT INTO testExecuteLargeBatch (n) VALUES (9), (10)");
				try {
					stmt.executeLargeBatch();
					System.out.println("error, should raise exception");
				} catch (BatchUpdateException e) {
					String err = "Incorrect int value: 'eight' for column 'n' at row 1";
					if (!err.equals(e.getMessage())) {
						System.out.println("wrong error message: " + e.getMessage());
					}
				}
				System.out.println("finished");
				con.close();
		}catch(Exception e) { System.out.println(e); }
	}
}

when plan cache is enabled, the error message returned is wrong: Incorrect int value: '4' for column 'n' at row 1, you can use this for diagnosing.

@dbjoa
Copy link
Contributor Author

dbjoa commented Oct 30, 2018

@eurekaka, Thank you for the test case. I'll come back after fixing the failure.

@dbjoa
Copy link
Contributor Author

dbjoa commented Oct 31, 2018

@eurekaka, The updated PR should fix the test failure.

@dbjoa
Copy link
Contributor Author

dbjoa commented Oct 31, 2018

/run-all-tests

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, except for the parser part mentioned. @zz-jason @winoros @lamxTyler PTAL.

@dbjoa
Copy link
Contributor Author

dbjoa commented Oct 31, 2018

@eurekaka, I've made the parser PR #5.

@@ -192,6 +193,7 @@ func (e *Execute) getPhysicalPlan(ctx sessionctx.Context, is infoschema.InfoSche
if err != nil {
return nil, errors.Trace(err)
}
log.Errorf("hit: %+v", ToString(plan))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why here use the level of “Error”?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the comment. The PR should remove the debugging code. I'll delete the line from the updated PR.

@@ -21,7 +21,12 @@ import (

// Cacheable checks whether the input ast is cacheable.
func Cacheable(node ast.Node) bool {
if _, isSelect := node.(*ast.SelectStmt); !isSelect {

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we remove this blank line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the comment. I'll delete the line from the updated PR.

@dbjoa dbjoa changed the title planner,executor,parser: support the plan cache for prepared insert/delete/update statements [WIP] planner,executor,parser: support the plan cache for prepared insert/delete/update statements Nov 1, 2018
@dbjoa
Copy link
Contributor Author

dbjoa commented Nov 2, 2018

/run-all-tests

@dbjoa dbjoa changed the title [WIP] planner,executor,parser: support the plan cache for prepared insert/delete/update statements planner,executor,parser: support the plan cache for prepared insert/delete/update statements Nov 2, 2018
@dbjoa
Copy link
Contributor Author

dbjoa commented Nov 2, 2018

@eurekaka, @zimulala PTAL

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 eurekaka added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 2, 2018
Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

Really great work! Rest LGTM

expression/util.go Show resolved Hide resolved
Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

LGTM

@XuHuaiyu XuHuaiyu added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. sig/planner SIG: Planner status/LGT2 Indicates that a PR has LGTM 2. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants