Skip to content

Conversation

jackysp
Copy link
Member

@jackysp jackysp commented Aug 24, 2018

What problem does this PR solve?

setCharacterStream(1, new StringReader(""), 0); this API in JDBC will use ComStmtSendLongData to send a long string. When the data length is 0, TiDB cannot distinguish whether there is a parameter with no data or there is no parameter at all.

What is changed and how it works?

This pr appends an extra 0 at the end of the bound parameter. In ComStmtExecute, we use it to distinguish if there is a parameter or not.
(How does MySQL deal with this?
MySQL set the length of the parameter as len(data - 6), which 6 is the length of the header of the request.)

Check List

Tests

  • Integration test (JDBC test cases)

Code changes

  • Has exported function/method change

Side effects

  • Increased code complexity

Related changes

  • Need to cherry-pick to the release branch

PTAL @coocood

Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala zimulala added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 24, 2018
winkyao
winkyao previously approved these changes Aug 25, 2018
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

@jackysp
Copy link
Member Author

jackysp commented Aug 27, 2018

/run-all-tests

@coocood
Copy link
Member

coocood commented Aug 27, 2018

@jackysp

This is a way to solve the problem, but may not the best.
The problem is that a nil slice appending a zero-length slice is still nil slice.

We can make it work like this in TiDBStatement

	if len(data) == 0 {
		ts.boundParams[paramID] = []byte{}
	} else {
		ts.boundParams[paramID] = append(ts.boundParams[paramID], data...)
	}

@jackysp
Copy link
Member Author

jackysp commented Aug 27, 2018

/run-all-tests

@jackysp jackysp added status/all tests passed and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 27, 2018
@jackysp
Copy link
Member Author

jackysp commented Aug 27, 2018

PTAL @coocood

@jackysp jackysp added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 27, 2018
@coocood
Copy link
Member

coocood commented Aug 27, 2018

LGTM

@coocood coocood merged commit 09fb68a into pingcap:master Aug 27, 2018
jackysp added a commit to jackysp/tidb that referenced this pull request Aug 28, 2018
@jackysp jackysp deleted the fix_binary_long_data branch September 7, 2018 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/mysql-protocol status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants