Skip to content

server: support for client multi-statement option (#19459)#20408

Merged
ti-srebot merged 4 commits intopingcap:release-4.0from
ti-srebot:release-4.0-b892fd8391d8
Nov 6, 2020
Merged

server: support for client multi-statement option (#19459)#20408
ti-srebot merged 4 commits intopingcap:release-4.0from
ti-srebot:release-4.0-b892fd8391d8

Conversation

@ti-srebot
Copy link
Contributor

cherry-pick #19459 to release-4.0


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.

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot ti-srebot added compatibility-breaker Violation of forwards/backwards compatibility in a design-time piece. component/server type/4.0-cherry-pick labels Oct 12, 2020
@ti-srebot ti-srebot requested review from jackysp and lysu October 12, 2020 05:34
@ti-srebot ti-srebot added this to the v4.0.8 milestone Oct 12, 2020
@ti-srebot ti-srebot assigned ghost Oct 12, 2020
@jackysp
Copy link
Contributor

jackysp commented Oct 13, 2020

/run-all-tests

@ghost
Copy link

ghost commented Oct 13, 2020

It looks like it needs tidb-test PR 1090 cherry picked to the release branch as well.

@ghost
Copy link

ghost commented Oct 13, 2020

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

@lysu
Copy link
Contributor

lysu commented Oct 14, 2020

LGTM

@ti-srebot
Copy link
Contributor Author

@lysu, Thanks for your review, however we are sorry that your vote won't be count.

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
Copy link
Contributor Author

@lysu, Thanks for your review, however we are sorry that your vote won't be count.

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 Oct 14, 2020
@lysu lysu requested review from bb7133 and cfzjywxk October 28, 2020 10:57
@bb7133
Copy link
Member

bb7133 commented Oct 28, 2020

The release-4.0 branch is frozen for now, let's hold it until merging is available again.

@ti-srebot
Copy link
Contributor Author

@lysu, @jackysp, @bb7133, @cfzjywxk, PTAL.

1 similar comment
@ti-srebot
Copy link
Contributor Author

@lysu, @jackysp, @bb7133, @cfzjywxk, PTAL.

@bb7133 bb7133 modified the milestones: v4.0.8, v4.0.9 Nov 4, 2020
@ti-srebot
Copy link
Contributor Author

@lysu, @jackysp, @bb7133, @cfzjywxk, PTAL.

@jackysp
Copy link
Contributor

jackysp commented Nov 5, 2020

/merge

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

Your auto merge job has been accepted, waiting for:

  • 20519

@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot
Copy link
Contributor Author

@ti-srebot merge failed.

@ghost
Copy link

ghost commented Nov 5, 2020

/run-unit-test

@jackysp
Copy link
Contributor

jackysp commented Nov 6, 2020

/merge

@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot ti-srebot merged commit 0b0c41b into pingcap:release-4.0 Nov 6, 2020
bb7133 added a commit to bb7133/tidb that referenced this pull request Jan 12, 2021
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/LGT1 Indicates that a PR has LGTM 1. type/4.0-cherry-pick

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants