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

server: support for client multi-statement option #19459

Merged
merged 10 commits into from Oct 12, 2020
Merged

server: support for client multi-statement option #19459

merged 10 commits into from Oct 12, 2020

Conversation

ghost
Copy link

@ghost ghost commented Aug 26, 2020

What problem does this PR solve?

Problem Summary:

The server previously did not correctly support the multi-statement client feature, which meant that multi-statement was always enabled. With this PR now it could be enabled or disabled (up to the client!)

What is changed and how it works?

What's Changed:

How it Works:

Related changes

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)

The go-sql driver allows changing the multi-statement option as a connection parameter, and I have added this to the test suite. I have also manually tested against the MySQL C client, which many drivers are based on. It allows changing the options of an open connection, and TiDB correctly supports this RPC. Here is that example program:

/*
* gcc example.c -o example  `mysql_config --cflags --libs`
*/
#include <mysql.h>
#include <stdio.h>
#include <string.h>

void run_single_test(MYSQL *conn) {

  if (mysql_query(conn, "update t1 set a = 1 where b = 1")) {
    printf("ERROR: %s\n", mysql_error(conn));
  } else {
    printf("SUCCESS: %s\n", mysql_error(conn));
  }

}

void run_multi_test(MYSQL *conn) {

  if (mysql_query(conn, "update t1 set a = 1 where b = 1;update t1 set a = 2 where b = 2;update t1 set a = 3 where b = 3;")) {
    printf("ERROR: %s\n", mysql_error(conn));
  } else {
    printf("SUCCESS: %s\n", mysql_error(conn));
  }

}

int main(int argc, char **argv) {

  MYSQL *conn = mysql_init(NULL);

  if (conn == NULL)  {
      fprintf(stderr, "%s\n", mysql_error(conn));
      exit(1);
  }

  if (mysql_real_connect(conn, "127.0.0.1", "root", "", "test", 4000, NULL, 0) == NULL) {
//   if (mysql_real_connect(conn, "127.0.0.1", "msandbox", "msandbox", "test", 8021, NULL, 0) == NULL) {
      fprintf(stderr, "%s\n", mysql_error(conn));
      mysql_close(conn);
      exit(1);
  }

  run_single_test(conn); // works
  run_multi_test(conn); // fails
  mysql_set_server_option(conn, MYSQL_OPTION_MULTI_STATEMENTS_ON);
  run_single_test(conn); // works
  run_multi_test(conn); // works

  mysql_close(conn);
  exit(0);

}

Here is the output of the program (whether or not the tables exist is inconsequential):

nullnotnil@ubuntu:~$ gcc example.c -o example  `mysql_config --cflags --libs` && ./example 
ERROR: Table 'test.t1' doesn't exist
ERROR: client has multi-statement capability disabled
ERROR: Table 'test.t1' doesn't exist
ERROR: Table 'test.t1' doesn't exist

Side effects

  • Breaking backward compatibility

Some clients may incorrectly be depending on the current behavior! It may break for them.

Release note

  • The TiDB Server previously did not correctly support disabling client multi-statement support.

@ghost ghost requested review from lysu and jackysp August 26, 2020 05:24
@ghost ghost added the component/server label Aug 26, 2020
@jackysp jackysp added the compatibility-breaker Violation of forwards/backwards compatibility in a design-time piece. label Aug 26, 2020
@jackysp
Copy link
Member

jackysp commented Aug 26, 2020

Which released branches should this PR backported to?

@ghost
Copy link
Author

ghost commented Aug 26, 2020

Which released branches should this PR backported to?

Maybe we can ask the DBA team? I think 3.1/4.0, but I do have some concern apps built directly for TiDB (and not MySQL) may be using the client libraries incorrectly.

Copy link
Member

@jackysp jackysp 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 Aug 27, 2020
@jackysp
Copy link
Member

jackysp commented Aug 27, 2020

Which released branches should this PR backported to?

Maybe we can ask the DBA team? I think 3.1/4.0, but I do have some concern apps built directly for TiDB (and not MySQL) may be using the client libraries incorrectly.

I think we need to cherry-pick it to 4.0 only.

@ghost
Copy link
Author

ghost commented Aug 27, 2020

I think we need to cherry-pick it to 4.0 only.

Sounds good to me

Copy link
Contributor

@lysu lysu 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 removed the status/LGT1 Indicates that a PR has LGTM 1. label Sep 2, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 2, 2020
@lysu
Copy link
Contributor

lysu commented Sep 2, 2020

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 2, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@nullnotnil merge failed.

@zz-jason
Copy link
Member

zz-jason commented Oct 4, 2020

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 20284

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@nullnotnil merge failed.

@ghost
Copy link
Author

ghost commented Oct 10, 2020

/run-all-tests

@ghost
Copy link
Author

ghost commented Oct 10, 2020

It looks like these might be real failures:

2020-10-10T04:40:08.470Z] [INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.064 s - in testsuite.regression.SyntaxRegressionTest

[2020-10-10T04:40:08.727Z] [INFO] 

[2020-10-10T04:40:08.727Z] [INFO] Results:

[2020-10-10T04:40:08.727Z] [INFO] 

[2020-10-10T04:40:08.727Z] [ERROR] Errors: 

[2020-10-10T04:40:08.727Z] [ERROR]   MultiQueryTest.testMultiQuery:57 ? SQL client has multi-statement capability d...

[2020-10-10T04:40:08.727Z] [INFO] 

[2020-10-10T04:40:08.727Z] [ERROR] Tests run: 296, Failures: 0, Errors: 1, Skipped: 0

[2020-10-10T04:40:08.727Z] [INFO] 

And:

[2020-10-10T04:35:36.152Z] /home/jenkins/agent/workspace/tidb_ghpr_integration_campatibility_test/go
[2020-10-10T04:36:11.181Z] 2020/10/10 12:36:10 Error 1105: client has multi-statement capability disabled
[2020-10-10T04:36:11.181Z] compatibletest check new failed

I'll take a look.

@ghost
Copy link
Author

ghost commented Oct 10, 2020

/run-all-tests tidb-test=pr/1090

@ghost ghost mentioned this pull request Oct 12, 2020
9 tasks
@jackysp
Copy link
Member

jackysp commented Oct 12, 2020

@jackysp
Copy link
Member

jackysp commented Oct 12, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@nullnotnil merge failed.

@jackysp
Copy link
Member

jackysp commented Oct 12, 2020

/run-unit-test

@lysu
Copy link
Contributor

lysu commented Oct 12, 2020

/run-unit-test tidb-test=pr/1090

@jackysp
Copy link
Member

jackysp commented Oct 12, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit b892fd8 into pingcap:master Oct 12, 2020
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Oct 12, 2020
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #20408

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility-breaker Violation of forwards/backwards compatibility in a design-time piece. component/server 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.

4 participants