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

Increase default maxAllowedPacket size. #1411

Merged
merged 2 commits into from Apr 14, 2023

Conversation

methane
Copy link
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
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
39 of 68 checks passed
@methane methane deleted the errPacketTooLarge branch April 14, 2023 10:00
@agnivade
Copy link

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

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

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
Member Author

methane commented May 24, 2023

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

@sjmudd
Copy link
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