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

strict mode causes mariadb 10.3.0 db connections to fail #635

Closed
jmhodges opened this issue Jul 11, 2017 · 13 comments
Closed

strict mode causes mariadb 10.3.0 db connections to fail #635

jmhodges opened this issue Jul 11, 2017 · 13 comments
Labels
Milestone

Comments

@jmhodges
Copy link
Contributor

jmhodges commented Jul 11, 2017

Using current HEAD (bf7f34f), code that enables Config.Strict will fail when trying to send queries to a mariadb 10.3.0 database. It returns Error 1158: Got an error reading communication packets.

	cfg, err := mysql.ParseDSN(dsn)
	if err != nil {
		return nil, fmt.Errorf("dbhelp.Open: unable to parse MySQL DSN: %s", err)
	}
	cfg.Strict = true
	dbDSN := cfg.FormatDSN()
	db, err := sql.Open("mysql", dbDSN)
	if err != nil {
		log.Fatalf("Open: %s", err)
	}
	err = db.Ping()
	if err != nil {
		log.Fatalf("Ping: %s", err)
	}
@jmhodges
Copy link
Contributor Author

10.2.6 fails, mostly, as well. Like the first will work, but running the code again will cause it to fail reliably

@jmhodges
Copy link
Contributor Author

10.1.25 works! So, there's something between the 10.1.25 and 10.2.6.

@methane
Copy link
Member

methane commented Jul 11, 2017

Please stop using strict mode.
It is known to fail.
see also: #609 (comment)

@jmhodges
Copy link
Contributor Author

That's not an option for dealing with mysql databases.

@jmhodges
Copy link
Contributor Author

jmhodges commented Jul 11, 2017

To be more clear: there is a large number of significant issues that are never caught if you do not enable strict mode and strict mode is considered to be a thing you should always turn on. There is no way we could use this library without it.

@jsha
Copy link

jsha commented Jul 11, 2017

I think there may be some confusion here between go-sql-driver's "strict" option vs MariaDB's SET sql_mode='STRICT_ALL_TABLES. I think it's the former that @methane is proposing to remove, and the latter that is considered best practice among MySQL / MariaDB developers.

See this, from go-sql-driver's README: https://github.com/go-sql-driver/mysql/blob/master/README.md

strict
Type: bool
Valid Values: true, false
Default: false
strict=true enables a driver-side strict mode in which MySQL warnings are treated as errors. This mode should not be used in production as it may lead to data corruption in certain situations.
A server-side strict mode, which is safe for production use, can be set via the sql_mode system variable.

@jmhodges
Copy link
Contributor Author

jmhodges commented Jul 11, 2017

No, that behavior of turning warnings into errors is common to MySQL client libraries and is recommended practice.

@jmhodges
Copy link
Contributor Author

jmhodges commented Jul 11, 2017

(There's also no way in most versions of Go for a user of database/sql to create a SET that's used in all conns in an efficient way, so that task has been left to the sql drivers to implement.)

(For folks listening at home and confused by that: that's because sql.DB is actually a collection of connections under the hood, not just one. Combine this with the flakiness of multi-statements in database/sql and you've got a bad deal.)

@methane
Copy link
Member

methane commented Jul 11, 2017

No, that behavior of turning warnings into errors is common to MySQL client libraries and is recommended practice.

Any reference? I didn't think it's common or recommended.

@methane
Copy link
Member

methane commented Jul 12, 2017

(There's also no way in most versions of Go for a user of database/sql to create a SET that's used in all conns in an efficient way, so that task has been left to the sql drivers to implement.)

No way?

@jmhodges
Copy link
Contributor Author

jmhodges commented Jul 12, 2017

I did not know about the system variables deal. Sorry!

SET GLOBAL requires the mysql user to have very high permissions and affects everyone on the server including folks who might not be able to handle them, of course.

Just to make sure I understand the situation: does that mean the driver currently doesn't report warnings that mysql gives at EOF? Does that include errors in EOFs, too, if they were STRICT_ALL_TABLES'ed? I'm definitely not as familiar with the protocol as you.

Thanks!

@methane
Copy link
Member

methane commented Jul 12, 2017

Just to make sure I understand the situation: does that mean the driver currently doesn't report warnings that mysql gives at EOF?

When warning_count field is not 0, strict mode client query SHOW WARNINGS and return
the result as error, if succeed. But after EOF packet is not safe to query always. In this case,
connection is broken and curious errors happens.

Does that include errors in EOFs, too, if they were STRICT_ALL_TABLES'ed?

No. When error happens in servre-side, it is returned directly. No need to query SHOW WARNINGS.
It's safe, robust and recommended.

sql_mode=TRADITIONAL,NO_AUTO_VALUE_ON_ZERO,ONLY_FULL_GROUP_BY is recommended in Japanese community. It's known as "Kamipo's tranditional".

julienschmidt added a commit that referenced this issue Sep 29, 2017
@julienschmidt julienschmidt mentioned this issue Sep 29, 2017
5 tasks
@julienschmidt julienschmidt added this to the v1.4 milestone Sep 30, 2017
julienschmidt added a commit that referenced this issue Oct 3, 2017
* Remove strict mode

Fixes #556 #602 #635
Closes #609

* dsn: panic in case of strict mode
@julienschmidt
Copy link
Member

Fixed by #676

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants