Add a check to rows.Err after processing all rows #835
Conversation
Closes github#822. In go-sql-driver/mysql#1075, @acharis notes that the way the go-sql driver is written, query timeout errors don't get set in `rows.Err()` until _after_ a call to `rows.Next()` is made. Because this kind of error means there will be no rows in the result set, the `for rows.Next()` will never enter the for loop, so we must check the value of `rows.Err()` after the loop, and surface the error up appropriately.
|
i think we might want to do a bit more than this. but definitely happy to see that someone else cares about this! |
|
Absolutely! I'm very interested in getting this sorted out :D I think you're absolutely right we should do more, and I'm happy to try to tag-team checking the rest of gh-ost for where we're missing this with you. I also think that this change as-is is an unequivocal win for gh-ost's data integrity, so my preference would be if we don't get the time to do a full check of gh-ost within a couple weeks that we merge this and at least plug the one hole we already know about it. What do you think? |
|
with you 100% i think this PR is definitely a win and should be merged as-is as soon as possible. |
|
@acharis I think I've covered all the relevant cases here, but open to additional feedback! |
Change-Id: Ib2e56c02ea1f98a5a47392ebbeeb6e88feb28e3c
Closes #822.
In go-sql-driver/mysql#1075, @acharis notes
that the way the go-sql driver is written, query timeout errors don't
get set in
rows.Err()until after a call torows.Next()is made.Because this kind of error means there will be no rows in the result
set, the
for rows.Next()will never enter the for loop, so we mustcheck the value of
rows.Err()after the loop, and surface the error upappropriately.
I then went ahead and fixed up the error checking in the two remaining
places we use
db.Query, and validated I've covered all cases byrunning:
script/cibuildreturns with no formatting errors, build errors or unit test errors.