server: Fix compression protocol for larger packets | tidb-test=pr/2227

[dveeden]

What problem does this PR solve?

Issue Number: close #47152

All issues fixed by this PR:

• close Load data with --compress=true fails #47152
• close Send huge SQL with --compress=true fails #47157
• close Querying huge data with useCompress=true fails #47161

Problem Summary:

What is changed and how it works?

Check List

Tests

• Unit test
• Integration test
• Manual test (add detailed scripts or steps below)
• No need to test

  • I checked and no code files have been changed.

Side effects

• Performance regression: Consumes more CPU
• Performance regression: Consumes more Memory
• Breaking backward compatibility

Documentation

• Affects user behaviors
• Contains syntax changes
• Contains variable changes
• Contains experimental features
• Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

A bug in the handling of the compressed protocol for larger payloads (>=16M) has been fixed

[ti-chi-bot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[tiprow]

Hi @dveeden. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[dveeden]

/ok-to-test

[dveeden]

#!/bin/perl
use strict;
use DBI;
use v5.36.0;

$| = 1;

my @dsns = (
	'DBI:mysql:database=test;port=4000;host=127.0.0.1;mysql_read_timeout=5',
	'DBI:mysql:database=test;port=4000;host=127.0.0.1;mysql_read_timeout=5;mysql_compression=1'
);

foreach my $dsn (@dsns) {
	say "Testing with DSN: " . $dsn;
	my $dbh = DBI->connect($dsn, 'root', undef);

	my $sth = $dbh->prepare("select repeat('a', ?)");
	my $size = (2**24)-20;
	while ($size < (2**24)+10) {
		print "Trying " . $size;
		$sth->execute($size) or die "FAIL: query execution failed\n";
		while (my $ref = $sth->fetchrow_arrayref()) {
			if (length($$ref[0]) == $size) {
				print " OK\n";
			} else {
				print " FAIL: expected " . $size . " but got " . $$ref[0] . "\n";
			}
			$size++;
		}
	}
	$sth->finish;
}

With this PR:

$ perl ~/compress_large_issue47161.pl 
Testing with DSN: DBI:mysql:database=test;port=4000;host=127.0.0.1;mysql_read_timeout=5
Trying 16777196 OK
Trying 16777197 OK
Trying 16777198 OK
Trying 16777199 OK
Trying 16777200 OK
Trying 16777201 OK
Trying 16777202 OK
Trying 16777203 OK
Trying 16777204 OK
Trying 16777205 OK
Trying 16777206 OK
Trying 16777207 OK
Trying 16777208 OK
Trying 16777209 OK
Trying 16777210 OK
Trying 16777211 OK
Trying 16777212 OK
Trying 16777213 OK
Trying 16777214 OK
Trying 16777215 OK
Trying 16777216 OK
Trying 16777217 OK
Trying 16777218 OK
Trying 16777219 OK
Trying 16777220 OK
Trying 16777221 OK
Trying 16777222 OK
Trying 16777223 OK
Trying 16777224 OK
Trying 16777225 OK
Testing with DSN: DBI:mysql:database=test;port=4000;host=127.0.0.1;mysql_read_timeout=5;mysql_compression=1
Trying 16777196 OK
Trying 16777197 OK
Trying 16777198 OK
Trying 16777199 OK
Trying 16777200 OK
Trying 16777201 OK
Trying 16777202 OK
Trying 16777203 OK
Trying 16777204 OK
Trying 16777205 OK
Trying 16777206 OK
Trying 16777207 OK
Trying 16777208 OK
Trying 16777209 OK
Trying 16777210 OK
Trying 16777211 OK
Trying 16777212 OK
Trying 16777213 OK
Trying 16777214 OK
Trying 16777215 OK
Trying 16777216 OK
Trying 16777217 OK
Trying 16777218 OK
Trying 16777219 OK
Trying 16777220 OK
Trying 16777221 OK
Trying 16777222 OK
Trying 16777223 OK
Trying 16777224 OK
Trying 16777225 OK

Without this PR:

$ perl ~/compress_large_issue47161.pl 
Testing with DSN: DBI:mysql:database=test;port=4000;host=127.0.0.1;mysql_read_timeout=5
Trying 16777196 OK
Trying 16777197 OK
Trying 16777198 OK
Trying 16777199 OK
Trying 16777200 OK
Trying 16777201 OK
Trying 16777202 OK
Trying 16777203 OK
Trying 16777204 OK
Trying 16777205 OK
Trying 16777206 OK
Trying 16777207 OK
Trying 16777208 OK
Trying 16777209 OK
Trying 16777210 OK
Trying 16777211 OK
Trying 16777212 OK
Trying 16777213 OK
Trying 16777214 OK
Trying 16777215 OK
Trying 16777216 OK
Trying 16777217 OK
Trying 16777218 OK
Trying 16777219 OK
Trying 16777220 OK
Trying 16777221 OK
Trying 16777222 OK
Trying 16777223 OK
Trying 16777224 OK
Trying 16777225 OK
Testing with DSN: DBI:mysql:database=test;port=4000;host=127.0.0.1;mysql_read_timeout=5;mysql_compression=1
Trying 16777196 OK
Trying 16777197 OK
Trying 16777198 OK
Trying 16777199 OK
Trying 16777200 OK
Trying 16777201 OK
Trying 16777202 OK
Trying 16777203 OK
Trying 16777204 OK
Trying 16777205 OK
Trying 16777206 OK
Trying 16777207DBD::mysql::st execute failed: Lost connection to MySQL server during query at /home/dvaneeden/compress_large_issue47161.pl line 21.
FAIL: query execution failed

[dveeden]

/check-issue-triage-complete

[codecov]

Codecov Report

Merging #47495 (ee66779) into master (c416ca1) will increase coverage by 1.4561%.
Report is 5 commits behind head on master.
The diff coverage is 62.5000%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #47495        +/-   ##
================================================
+ Coverage   71.4979%   72.9540%   +1.4561%     
================================================
  Files          1402       1425        +23     
  Lines        406163     414018      +7855     
================================================
+ Hits         290398     302043     +11645     
+ Misses        95955      93108      -2847     
+ Partials      19810      18867       -943     

Flag	Coverage Δ
integration	43.0884% <5.2083%> (?)
unit	71.6428% <62.5000%> (+0.1449%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	53.9874% <ø> (ø)
parser	∅ <ø> (∅)
br	48.8714% <ø> (-4.0642%)	⬇️

[djshow832]

1. Can you add a unit test to prevent it from broken by others?
2. As mentioned in Querying huge data with useCompress=true fails #47161 (comment), what if the result set is larger than 256MB?

[dveeden]

1. Can you add a unit test to prevent it from broken by others?

Yes. That was why the PR was still in draft status.

2. As mentioned in [Querying huge data with useCompress=true fails #47161 (comment)](https://github.com/pingcap/tidb/issues/47161#issuecomment-1742322274), what if the result set is larger than 256MB?

That still needs to be fixed.

[djshow832]

I don't see how the second problem in #47152 (comment) is solved: sometimes uncompressed sequence is affected by compressed sequence.

[dveeden]

I don't see how the second problem in #47152 (comment) is solved: sometimes uncompressed sequence is affected by compressed sequence.

With MySQL 8.2.0:

Packet	Sequence	Compressed Sequence	Description
24	0	0	Query: LOAD DATA INFILE
25	1	1	LOCAL INFILE request
27	2,3	2	LOCAL INFILE response
28	3	3	OK

With TiDB (this PR):

Packet	Sequence	Compressed Sequence	Description
57	0	0	Query: LOAD DATA INFILE
58	1	1	LOCAL INFILE request
60	2,3	2	LOCAL INFILE response
61	4	3	OK

Here the sequence number 3 gets duplicated with MySQL, probably because in net_flush() for packet 27 the compressed packet number (2) gets synced to the regular packet number.

I'm not sure which one is best, but I also haven't seen any issues with this from a client standpoint. I had a quick look at the Connector/J code and there I didn't see packet numbers of the regular and compressed packets being synced.

[dveeden]

I don't see how the second problem in #47152 (comment) is solved: sometimes uncompressed sequence is affected by compressed sequence.

I've now done this in ee66779

[dveeden]

/retest

[dveeden]

/retest

[dveeden]

/test tiprow_fast_test

[ti-chi-bot]

@dveeden: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

• /test build
• /test check-dev
• /test check-dev2
• /test mysql-test
• /test unit-test

The following commands are available to trigger optional jobs:

• /test canary-notify-when-compatibility-sections-changed
• /test pingcap/tidb/canary_ghpr_unit_test
• /test pull-br-integration-test
• /test pull-common-test
• /test pull-e2e-test
• /test pull-integration-common-test
• /test pull-integration-copr-test
• /test pull-integration-ddl-test
• /test pull-integration-jdbc-test
• /test pull-integration-mysql-test
• /test pull-integration-nodejs-test
• /test pull-mysql-client-test
• /test pull-sqllogic-test
• /test pull-tiflash-test

Use /test all to run the following jobs that were automatically triggered:

• pingcap/tidb/ghpr_build
• pingcap/tidb/ghpr_check
• pingcap/tidb/ghpr_check2
• pingcap/tidb/ghpr_mysql_test
• pingcap/tidb/ghpr_unit_test

In response to this:

/test tiprow_fast_test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[tiprow]

@dveeden: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
tiprow_fast_test	ee66779	link	true	/test tiprow_fast_test

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: djshow832, lance6716

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [djshow832,lance6716]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ti-chi-bot]

[LGTM Timeline notifier]

Timeline:

• 2023-10-23 02:50:51.348395406 +0000 UTC m=+2230248.935505551: ☑️ agreed by lance6716.
• 2023-11-03 00:40:32.192536658 +0000 UTC m=+3172829.779646788: ☑️ agreed by djshow832.

[ti-chi-bot]

In response to a cherrypick label: new pull request created to branch release-7.5: #48255.

[ti-chi-bot]

In response to a cherrypick label: new pull request created to branch release-7.1: #48256.