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: use max_allowed_packet to limit the packet size. #33017

Closed
wants to merge 0 commits into from

Conversation

CbcWestwolf
Copy link
Member

@CbcWestwolf CbcWestwolf commented Mar 11, 2022

What problem does this PR solve?

Issue Number: close #31422

Problem Summary:

The max_allowed_packet exists but did not work on previous versions.

What is changed and how it works?

Check the length of data in dispatch function.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • 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

None

@ti-chi-bot
Copy link
Member

[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.

@ti-chi-bot ti-chi-bot added needs-cherry-pick-release-5.0 needs-cherry-pick-release-5.1 do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. needs-cherry-pick-release-5.2 needs-cherry-pick-release-5.3 Type: Need cherry pick to release-5.3 needs-cherry-pick-release-5.4 Should cherry pick this PR to release-5.4 branch. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 11, 2022
@sre-bot
Copy link
Contributor

sre-bot commented Mar 11, 2022

@CbcWestwolf CbcWestwolf marked this pull request as ready for review March 16, 2022 14:02
@ti-chi-bot ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 16, 2022
@CbcWestwolf CbcWestwolf marked this pull request as draft March 18, 2022 02:59
@ti-chi-bot ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-cherry-pick-6.0 labels Mar 18, 2022
@CbcWestwolf CbcWestwolf marked this pull request as ready for review March 18, 2022 03:11
@ti-chi-bot ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 18, 2022
@morgo morgo requested review from dveeden and mjonss March 22, 2022 03:22
Copy link
Contributor

@dveeden dveeden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On first glance this looks wrong to me. It looks like a full packet is read and then discarded if it is bigger than max_allowed_packet. I think the right thing is abort before reading a packet that is too large.

In readOnePacket() the reading is done in two steps, it first reads the header and then gets the length from the header. I think this is where we should check for max_allowed_packet.

https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_basic_packets.html#sect_protocol_basic_packets_packet for how the header looks.

Looks like data of >16MiB might be send with multiple packets. If that's indeed the case we should bail out as soon as the buffer is more than max_allowed_packet. But we should also verify what MySQL does in this case... does it allow say a 512M blob to be sent multiplexed over multiple 8MiB packets? https://dev.mysql.com/doc/c-api/8.0/en/mysql-stmt-send-long-data.html might allow one to do that.

@dveeden
Copy link
Contributor

dveeden commented Mar 31, 2022

Looks like the behavior between TiDB and MySQL 8.0 is slightly different:

[dvaneeden@dve-carbon ~]$ ./max_allowed_packet.py 
Using 127.0.0.1:4000
  Testing with REPEAT("x", 100)
  Testing with client side payload of 100)
  Testing with REPEAT("x", 8192)
  Testing with client side payload of 8192)
  Testing with REPEAT("x", 16384)
  Testing with client side payload of 16384)
    Insert failed: 2013 (HY000): Lost connection to MySQL server during query
  Database connection failure on ping(): Connection to MySQL is not available
  Forcing reconnection
  Testing with REPEAT("x", 3232768)
    Insert failed: 1301 (HY000): Result of repeat() was larger than max_allowed_packet (16385) - truncated
  Testing with client side payload of 3232768)
    Insert failed: 2013 (HY000): Lost connection to MySQL server during query
Using 127.0.0.1:8028
  Testing with REPEAT("x", 100)
  Testing with client side payload of 100)
  Testing with REPEAT("x", 8192)
  Testing with client side payload of 8192)
  Testing with REPEAT("x", 16384)
  Testing with client side payload of 16384)
    Insert failed: 1153 (08S01): Got a packet bigger than 'max_allowed_packet' bytes
  Database connection failure on ping(): Connection to MySQL is not available
  Forcing reconnection
  Testing with REPEAT("x", 3232768)
    Insert failed: 1301 (HY000): Result of repeat() was larger than max_allowed_packet (16384) - truncated
  Testing with client side payload of 3232768)
    Insert failed: 1153 (08S01): Got a packet bigger than 'max_allowed_packet' bytes

The contents of max_allowed_packet.py :

#!/bin/python3
import mysql.connector

configs = [
    {
        "host": "127.0.0.1",
        "port": 4000,
        "user": "root",
        "database": "test",
    },
    {
        "host": "127.0.0.1",
        "port": 8028,
        "user": "msandbox",
        "password": "msandbox",
        "database": "test",
    },
]

for config in configs:
    print(f"Using {config['host']}:{config['port']}")
    con = mysql.connector.connect(**config)
    c = con.cursor()
    c.execute("DROP TABLE IF EXISTS t1")
    c.execute(
        "CREATE TABLE t1 (id BIGINT UNSIGNED AUTO_INCREMENT PRIMARY KEY, payload LONGBLOB)"
    )
    c.execute("SET GLOBAL max_allowed_packet=16385")

    for size in [100, 8192, 16384, 3232768]:
        try:
            con.ping()
        except mysql.connector.Error as e:
            print(f"  Database connection failure on ping(): {e}")
            print(f"  Forcing reconnection")
            con.ping(reconnect=True)

        try:
            print(f'  Testing with REPEAT("x", {size})')
            c.execute(f'INSERT INTO t1(payload) VALUES (REPEAT("x", {size}))')
            con.commit()
        except mysql.connector.Error as e:
            print(f"    Insert failed: {e}")

        try:
            con.ping()
        except mysql.connector.Error as e:
            print(f"  Database connection failure on ping(): {e}")
            print(f"  Forcing reconnection")
            con.ping(reconnect=True)

        try:
            print(f"  Testing with client side payload of {size})")
            c.execute("INSERT INTO t1(payload) VALUES(%s)", ("y" * size,))
            con.commit()
        except mysql.connector.Error as e:
            print(f"    Insert failed: {e}")

    c.close()
    con.close()

@dveeden
Copy link
Contributor

dveeden commented Mar 31, 2022

With MySQL 8.0.28:

mysql> set global max_allowed_packet=16385;
Query OK, 0 rows affected, 1 warning (0.00 sec)

mysql> show warnings;
+---------+------+-------------------------------------------------------+
| Level   | Code | Message                                               |
+---------+------+-------------------------------------------------------+
| Warning | 1292 | Truncated incorrect max_allowed_packet value: '16385' |
+---------+------+-------------------------------------------------------+
1 row in set (0.00 sec)

mysql> select @@global.max_allowed_packet;
+-----------------------------+
| @@global.max_allowed_packet |
+-----------------------------+
|                       16384 |
+-----------------------------+
1 row in set (0.00 sec)

@ti-chi-bot ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 1, 2022
@CbcWestwolf CbcWestwolf closed this Apr 1, 2022
@ti-chi-bot ti-chi-bot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 1, 2022
@ti-chi-bot ti-chi-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 1, 2022
@CbcWestwolf CbcWestwolf deleted the max_allowed_packet branch April 1, 2022 07:46
@CbcWestwolf CbcWestwolf restored the max_allowed_packet branch April 1, 2022 07:46
@CbcWestwolf CbcWestwolf deleted the max_allowed_packet branch April 1, 2022 07:47
@CbcWestwolf
Copy link
Member Author

@dveeden This PR is closed when I did a force-push operation. So I create another PR #33651 with the same code.

Now the truncation of max_allowed_packet is implemented. And the behavior between TiDB and MySQL 8.0 using your Python script is the same.

PTAL. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-cherry-pick-release-5.0 needs-cherry-pick-release-5.1 needs-cherry-pick-release-5.2 needs-cherry-pick-release-5.3 Type: Need cherry pick to release-5.3 needs-cherry-pick-release-5.4 Should cherry pick this PR to release-5.4 branch. release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

max_allowed_packet length is set to 1024 but its not working for string length.
4 participants