server: compare packet length with variable max_allowed_packet

[unconsolable]

Signed-off-by: unconsolable [email protected]

What problem does this PR solve?

Issue Number: close #31422 #32164

Problem Summary:

Session variable max_allowed_packet seems not taking effect

What is changed and how it works?

• According to https://dev.mysql.com/doc/refman/8.0/en/packet-too-large.html, packet length is compared with session variable max_allowed_packet before dispatch. If packet is too large, corresponding error is returned and connection is closed.
• It is a bit hard to write an UT for it, so a manual test is chosen instead as follows.

Check List

Tests

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

  set @@net_buffer_length=1024;
  set @@max_allowed_packet=1024;
  SELECT length("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa") as len;

• No code

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

Session variable `max_allowed_packet` will take effect and compare with packet length

[ti-chi-bot]

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

[sre-bot]

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/e29fcf234750f9954e53f611601e5f0b767d28ec

[Defined2014]

Could you add a test, like an IT test?

[unconsolable]

I tried to add an IT in explaintest in commit 102032d, but it failed in test new_character_set as follows https://ci.pingcap.net/blue/organizations/jenkins/tidb_ghpr_check/detail/tidb_ghpr_check/49418/pipeline. So I revert it.

[Defined2014]

The data maybe not only contain one packet, see the code below.

tidb/server/packetio.go

Lines 128 to 139 in cb8e65a

	for {
	buf, err := p.readOnePacket()
	if err != nil {
	return nil, errors.Trace(err)
	}
	data = append(data, buf...)
	if len(buf) < mysql.MaxPayloadLen {
	break
	}
	}

And the scope of max_allowed_packet in TiDB also have some problems.
In MySQL

mysql> set @@max_allowed_packet=default;
ERROR 1621 (HY000): SESSION variable 'max_allowed_packet' is read-only. Use SET GLOBAL to assign the value

[unconsolable]

• From https://dev.mysql.com/doc/refman/8.0/en/packet-too-large.html

A communication packet is a single SQL statement sent to the MySQL server, a single row that is sent to the client, or a binary log event sent from a replication source server to a replica.

So I think the packet of readOnePacket is different from the communication packet, but I'm not that sure. We'd better wait on others' opinion.

• The scope does has some problem, but I think we'd better leave for another issue.

[unconsolable]

I think it is not a suitable fix, as it only compare the packet length, without reducing buffer allocation according to max_allowed_packet, so it will be closed.