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

avoid returning empty MySQLWarnings in odd edge case scenario #555

Closed

Conversation

rubensayshi
Copy link

@rubensayshi rubensayshi commented Mar 22, 2017

Description

Was running into a weird edge case where the first 'row' was already EOF and result was empty MySQLWarnings{} when in strict=true mode.

Unfortunately unable to figure out what is really going on or reproduce it in isolation from main project.

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

@julienschmidt
Copy link
Member

Makes sense to me, but it would be nice if you could provide us an example for the edge case, so we can use it as a regression test.

Moreover, without an entry to the AUTHORS file we can not merge anything.

@julienschmidt julienschmidt added this to the v1.4 milestone Mar 23, 2017
@julienschmidt
Copy link
Member

Please keep the name list on AUTHORS in alphabetical order.

Can you also provide details about the edge case?

@@ -49,6 +49,7 @@ Xiangyu Hu <xiangyu.hu at outlook.com>
Xiaobing Jiang <s7v7nislands at gmail.com>
Xiuming Chen <cc at cxm.cc>
Zhenye Xie <xiezhenye at gmail.com>
Ruben de Vries <ruben at rubensayshi.com>
Copy link
Member

Choose a reason for hiding this comment

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

Please keep the name list in alphabetical order

@rubensayshi
Copy link
Author

rubensayshi commented May 10, 2017

@julienschmidt I think I managed to figure out when it occurs

schema.sql

DROP TABLE IF EXISTS `table1`;
CREATE TABLE `table1` (
  `id` INT(11) UNSIGNED NOT NULL,
  PRIMARY KEY (`id`)
);

DROP TABLE IF EXISTS `table2`;
CREATE TABLE `table2` (
  `id` INT(11) UNSIGNED NOT NULL AUTO_INCREMENT,

  PRIMARY KEY (`id`)
);

fixtures.sql

INSERT INTO `table1` VALUES (71398);
INSERT INTO `table2` VALUES (71398);

execute these 2 with ?strict=true&multiStatements=true

I've been trying to look at the handling of the msgs from mysql, when loading the fixtures:

readResultSetHeaderPacket 0001000a000000 
==================
handleOkPacket id=5 data=0001000a000000 
mc.affectedRows=1 (01)(1) mc.insertId=0 (00)(1) 
handleOkPacket m=1 n=1 status=a 
handleOkPacket::discardResults... 
discardResults mc.status=10 discard=true 
readResultSetHeaderPacket 0001fde6160102000000 
==================
handleOkPacket id=6 data=0001fde6160102000000 
mc.affectedRows=1 (01)(1) mc.insertId=71398 (fde61601)(4) 
handleOkPacket m=4 n=1 status=2 
handleOkPacket::discardResults... 
discardResults mc.status=2 discard=false 
handleOkPacket::checkwarnings... id=6 data=0001fde6160102000000 n=1 m=4 pos=8 warningCnt=0000 warningCnt=0 
handleOkPacket::checkwarnings... id=5 data=0001fde6160102 n=1 m=1 pos=5 warningCnt=0102 warningCnt=513 
handleOkPacket::WARNINGS 513 
readResultSetHeaderPacket 03 
discardResults mc.status=2 discard=false 

see the handleOkPacket and handleOkPacket::checkwarnings... (checkwarning... is when it determines if it should do mc.getWarnings()).
the id= is some incrementing id I added each time handleOkPacket is called.
it seems the byte array data is different at the end of the id=5 call...

I'm not really sure what this code is supposed to do, how it works, just debugging, I hope you understand this stuff a little better xD

obviously this ain't the correct fix, but doing data := make([]byte, len(_data)); copy(data, _data) at the start of handleOkPacket fixes the issue (just to 'prove' it has something to do with the underlying byte array of the slice)

@rubensayshi
Copy link
Author

ping @julienschmidt

@rubensayshi
Copy link
Author

closing this, in favor of #602

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

Successfully merging this pull request may close these issues.

2 participants