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.

Sign up for GitHub

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

Jump to bottom

defer getWarnings() after fetching resultsets. #605

Closed
wants to merge 1 commit into from
Closed

defer getWarnings() after fetching resultsets. #605

wants to merge 1 commit into from

Conversation

methane
Copy link
Member

@methane methane commented May 30, 2017 •
edited
Loading

Description

Don't query "SHOW WARNINGS" in handleOkPacket(). Defer it to end of query.

fixes #602, maybe.

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

@coveralls
Copy link

coveralls commented May 30, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.2%) to 74.354% when pulling ca01562 on methane:fix-warnings into 382e13d on go-sql-driver:master.

julienschmidt
julienschmidt requested changes May 30, 2017
View reviewed changes
Copy link
Member

@julienschmidt julienschmidt left a comment

Choose a reason for hiding this comment

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

This never actually checks if strict mode is active

driver.go Outdated
@@ -175,6 +175,10 @@ func handleAuthResult(mc *mysqlConn, oldCipher []byte) error {
}
_, err = mc.readResultOK()
}

if err == nil && mc.warningCount > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't check if the strict mode is active

infile.go Outdated
@@ -175,6 +175,9 @@ func (mc *mysqlConn) handleInFileRequest(name string) (err error) {
// read OK packet
if err == nil {
_, err = mc.readResultOK()
if err == nil && mc.warningCount > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

same here

@julienschmidt
Copy link
Member

julienschmidt commented May 30, 2017 •
edited
Loading

We already know that the strict mode might lead to data corruption and this PR seems to make it even worse.
Further, it seems to be impossible to implement it correctly driver-side with multi-results.

How about we eventually completely remove the strict mode instead?

@methane
Copy link
Member Author

methane commented May 30, 2017

I haven't used use strict mode in Go.
But sometimes warning message is important.
For example, "LOAD DATA INFILE" produce warning message about where
MySQL server failed to parse CSV file.
Python's MySQL driver shows warnings by default.

I want to keep some way to check warning count and "SHOW WARNINGS".

@julienschmidt
Copy link
Member

How is the Python driver showing warnings by default? Does it send a SHOW WARNINGS query every time or does it simple set some server-side mode?

@methane
Copy link
Member Author

methane commented May 30, 2017

Like this pull request.
When multi statement, only warnings of last statement are shown.

@methane methane mentioned this pull request May 30, 2017
Closed
@methane methane force-pushed the fix-warnings branch 2 times, most recently from 67c0581 to 7a1a598 Compare June 1, 2017 11:22
@methane
Copy link
Member Author

methane commented Jun 1, 2017

hmm, why travis doesn't work?

@julienschmidt
Copy link
Member

I think Travis had some issues in the last few days. I restarted the job for this PR. Let's see if it works.

@julienschmidt
Copy link
Member

nope, that didn't work. Maybe add another commit (or amend and force-push the previous one) to trigger another build?

julienschmidt
julienschmidt reviewed Jun 5, 2017
View reviewed changes
connection.go
@@ -31,6 +31,7 @@ type mysqlConn struct {
sequence uint8
parseTime bool
strict bool
warningCount uint16
Copy link
Member

Choose a reason for hiding this comment

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

The fields are sorted by size for efficient struct packing. The warningCount field should moved up (before flags I think)

@julienschmidt julienschmidt modified the milestone: v1.4 Jun 6, 2017
@methane methane closed this Jun 6, 2017
@methane methane mentioned this pull request Jun 6, 2017
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julienschmidt julienschmidt julienschmidt requested changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
v1.4.0
Development

Successfully merging this pull request may close these issues.

Incorrect packet parsing for multiStatement=true
3 participants
@methane @coveralls @julienschmidt