Skip to content

server: support for client multi-statement option#19459

Merged
ti-srebot merged 10 commits intomasterfrom
unknown repository
Oct 12, 2020
Merged

server: support for client multi-statement option#19459
ti-srebot merged 10 commits intomasterfrom
unknown repository

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 jackysp and lysu 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
Contributor

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
Contributor

@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
Contributor

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
Contributor

jackysp commented Oct 12, 2020

@jackysp
Copy link
Contributor

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
Contributor

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
Contributor

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