Skip to content

Increase default maxAllowedPacket size.#1411

Merged
methane merged 2 commits intogo-sql-driver:masterfrom
methane:errPacketTooLarge
Apr 14, 2023
Merged

Increase default maxAllowedPacket size.#1411
methane merged 2 commits intogo-sql-driver:masterfrom
methane:errPacketTooLarge

Conversation

@methane
Copy link
Copy Markdown
Member

@methane methane commented Apr 11, 2023

Description

  • Increase default maxAllowedPacket as MySQL default.
  • ErrPacketTooLarge message mention client config rather than server config.

Fixes #1355.

Checklist

  • Code compiles correctly
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary
  • Added myself / the copyright holder to the AUTHORS file

@methane methane requested a review from shogo82148 April 11, 2023 12:06
Copy link
Copy Markdown
Contributor

@shogo82148 shogo82148 left a comment

Choose a reason for hiding this comment

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

LGTM

@methane methane merged commit f0e16c6 into go-sql-driver:master Apr 14, 2023
@methane methane deleted the errPacketTooLarge branch April 14, 2023 10:00
@agnivade
Copy link
Copy Markdown

FWIW we should have kept the default to be the same and let users change it via the query param. Changing the default to be higher is a breaking change for customers who have not changed their server side setting.

Please consider reverting this to be the original value of 4MB. This is causing a breaking change from our side: mattermost/mattermost#23394 (comment)

@methane
Copy link
Copy Markdown
Member Author

methane commented May 17, 2023

I'm sorry about changing it in 1.7.1. I had to wait 1.8.
But decreasing it is also backward breaking change. Strict zero backward incompatibility is impossible, because some value/behavior might be bug for some users but feature for another users.

Please advocate to use larger max_allowed_packet when user might send more than 4MB blob.

@agnivade
Copy link
Copy Markdown

Keeping the default to be the same, and letting users "fix" the issue by changing the query param would have been the best way to keep full backward compatibility and fix the issue as well.

And then if you want to change the default, cut a 2.0 release.

@methane
Copy link
Copy Markdown
Member Author

methane commented May 23, 2023

Keeping the default to be the same, and letting users "fix" the issue by changing the query param would have been the best way to keep full backward compatibility and fix the issue as well.

It may break people using v1.7.1 with default parameters.

Keeping the default to be the same, and letting users "fix" the issue by changing the query param would have been the best way to keep full backward compatibility and fix the issue as well.

And then if you want to change the default, cut a 2.0 release.

It is totally nonsense, like this comic.

We need to break small part every release, even in bugfix release.

We release v2 when we need to change/remove public APIs.

If you can not allow such a small behavior change, you must pin the exact version.

@agnivade
Copy link
Copy Markdown

It may break people using v1.7.1 with default parameters.

I don't see how. If the parameter is unset, it would still use the original value.

@methane
Copy link
Copy Markdown
Member Author

methane commented May 24, 2023

I don't understand what you're saying...

@sjmudd
Copy link
Copy Markdown
Contributor

sjmudd commented Jun 14, 2023

@agnivade : Also FWIW as I brought this up a 4MB query size is tiny. The original problem was the fact that the go driver error message said the server's setting was wrong (it wasn't) and also the 4 MB default size is tiny. This has nothing to do with blobs, just large queries can easily grow over this size.

  • MySQL 5.7 will be EOL in October and is very old. The setting dates back to 5.6 in 2013 where it moved from 1 MB to 4 MB
  • MariaDB uses 16 MB by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ErrPktTooLarge message may be incorrect

4 participants