expression: check max_allowed_packet constraint for function insert #7502

eurekaka · 2018-08-27T12:05:29Z

What problem does this PR solve?

Return NULL and a warning when the result of function insert exceeds max_allowed_packet.

Before this PR, SELECT INSERT('abc', 1, -1, 'abcd'); would return result 'abcd' even if @@global.max_allowed_packet is set 3.

After this PR:

mysql> select @@max_allowed_packet;
+----------------------+
| @@max_allowed_packet |
+----------------------+
| 3                    |
+----------------------+
1 row in set (0.00 sec)

mysql> select insert('abc', 1, -1, 'abc');
+-----------------------------+
| insert('abc', 1, -1, 'abc') |
+-----------------------------+
| abc                         |
+-----------------------------+
1 row in set (0.00 sec)

mysql> select insert('abc', 1, -1, 'abcd');
+------------------------------+
| insert('abc', 1, -1, 'abcd') |
+------------------------------+
| NULL                         |
+------------------------------+
1 row in set, 1 warning (0.00 sec)

mysql> show warnings;
+---------+------+-----------------------------------------------------------------------+
| Level   | Code | Message                                                               |
+---------+------+-----------------------------------------------------------------------+
| Warning | 1301 | Result of insert() was larger than max_allowed_packet (3) - truncated |
+---------+------+-----------------------------------------------------------------------+
1 row in set (0.00 sec)

What is changed and how it works?

check result length in valString method of builtinInsertSig and builtinInsertBinarySig against max_allowed_packet;

Check List

Tests

Unit test

Related changes

Need to be included in the release note

CLAassistant · 2018-08-27T12:05:37Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ eurekaka
✅ zz-jason
❌ Kenan Yao

Kenan Yao seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

eurekaka · 2018-08-27T12:17:11Z

Tests green, please take a look at this, thx @zz-jason @winoros @XuHuaiyu

zz-jason · 2018-08-27T12:15:35Z

expression/builtin_string.go

+		if uint64(pos-1+int64(len(newstr)))*uint64(mysql.MaxBytesOfCharacter) > b.maxAllowedPacket {
+			b.ctx.GetSessionVars().StmtCtx.AppendWarning(errWarnAllowedPacketOverflowed.GenByArgs("insert", b.maxAllowedPacket))
+			return "", true, nil
+		}
 		return str[0:pos-1] + newstr, false, nil
 	}
 	return str[0:pos-1] + newstr + str[pos+length-1:], false, nil


should we check whether len(str[0:pos-1] + newstr + str[pos+length-1:]) > b. maxAllowedPacket ?

zz-jason · 2018-08-27T12:17:33Z

expression/builtin_string.go

@@ -3216,18 +3225,24 @@ func (b *builtinInsertBinarySig) evalString(row chunk.Row) (string, bool, error)
 	}

 	if length > strLength-pos+1 || length < 0 {
+		if uint64(pos-1+int64(len(newstr)))*uint64(mysql.MaxBytesOfCharacter) > b.maxAllowedPacket {


function len() returns the number of bytes the string contains, no need to multiple mysql.MaxBytesOfCharacter.

zz-jason · 2018-08-27T12:19:06Z

to #7153

- compute bytes resulted correctly; - cover more cases of insert parameters;

zz-jason · 2018-08-28T08:13:11Z

LGTM

zz-jason · 2018-08-28T08:13:18Z

/run-all-tests

zz-jason · 2018-08-28T08:39:07Z

/run-integration-common-test
/run-common-test

zz-jason · 2018-08-28T08:49:30Z

@winoros @lamxTyler PTAL

alivxxx · 2018-08-28T11:26:18Z

expression/builtin_string.go

+		length = runeLength - pos + 1
+	}
+
+	if uint64(runeLength-length)*uint64(mysql.MaxBytesOfCharacter)+uint64(len(newstr)) > b.maxAllowedPacket {


What if the number of bytes of some character is less than MaxBytesOfCharacter? Will this raise warning while it should not?

Yes, this false-negative case exists, but to compute the exact bytes of these characters, we may need the charset info of the string, and then compute the length of specific characters for this charset, which incurs too much overhead, I guess that is why we use MaxBytesOfCharacter in other functions such as builtinLpadSig::evalString. @zz-jason should we compute the exact bytes for the string?

maybe we can build the string first, and use len() to compute the consumed bytes, but is it possible that panic is raised in building the string because it is too large? I think the purpose of max_allowed_packet check here is to prevent this kind of panic to some extent.

in MySQL, the byte count for lpad is calculated by:

byte_count = count * collation.collation->mbmaxlen;

as for the insert function, seems MySQL calculates the exact bytes for the string:

/* There is one exception not handled (intentionaly) by the character set aggregation code. If one string is strong side and is binary, and another one is weak side and is a multi-byte character string, then we need to operate on the second string in terms on bytes when calling ::numchars() and ::charpos(), rather than in terms of characters. Lets substitute its character set to binary. */ if (collation.collation == &my_charset_bin) { res->set_charset(&my_charset_bin); res2->set_charset(&my_charset_bin); } /* start and length are now sufficiently valid to pass to charpos function */ start = res->charpos((int)start); length = res->charpos((int)length, (uint32)start);

Why first build it? It just constitutes of three parts, so sum them would be enough.

seems there isn't a convenient and efficient way to calculate the byte count of a []rune. @tiancaiamao any idea?

another method is to write a utf-8 code point iterator like this: https://gist.github.com/zz-jason/078110974bb931b7f8e3432775ecfd05, we can iterate on the origin []byte, find the code point located at pos and pos+length, and the count the bytes for each []rune prefix.

oh you are right... updated

eurekaka · 2018-08-28T12:42:11Z

/run-all-tests

eurekaka · 2018-08-28T12:49:39Z

/run-all-tests

eurekaka · 2018-08-29T01:24:13Z

/run-all-tests

iamxy · 2018-08-29T02:57:54Z

/rebuild

iamxy · 2018-08-29T03:02:58Z

/run-all-tests

eurekaka · 2018-08-29T04:46:55Z

/run-all-tests

eurekaka · 2018-08-29T05:44:25Z

tests green, @zz-jason , @lamxTyler PTAL, thx

zz-jason

LGTM

alivxxx

LGTM

…ingcap#7502)

…7502) (#7528)

expression: check max_allowed_packet constraint for function insert

086bbaf

eurekaka requested a review from zz-jason August 27, 2018 12:17

zz-jason reviewed Aug 27, 2018

View reviewed changes

zz-jason mentioned this pull request Aug 27, 2018

need to handle "ER_WARN_ALLOWED_PACKET_OVERFLOWED" for some string functions #7153

Open

13 tasks

zz-jason added type/bugfix This PR fixes a bug. component/expression labels Aug 27, 2018

eurekaka and others added 2 commits August 28, 2018 16:04

modifications according to comments:

04fa228

- compute bytes resulted correctly; - cover more cases of insert parameters;

Merge branch 'master' into str_udf_overflow_insert

558bc71

zz-jason added status/LGT1 Indicates that a PR has LGTM 1. status/all tests passed labels Aug 28, 2018

alivxxx reviewed Aug 28, 2018

View reviewed changes

Kenan Yao and others added 2 commits August 28, 2018 20:08

Merge branch 'master' into str_udf_overflow_insert

28a5ba4

calcute accurate bytes consumed by the result string

99faa60

Merge branch 'master' into str_udf_overflow_insert

b27aa83

zz-jason reviewed Aug 29, 2018

View reviewed changes

alivxxx approved these changes Aug 29, 2018

View reviewed changes

alivxxx merged commit c625c27 into pingcap:master Aug 29, 2018

eurekaka deleted the str_udf_overflow_insert branch August 29, 2018 05:57

eurekaka added the type/2.0 cherry-pick label Aug 29, 2018

eurekaka added a commit to eurekaka/tidb that referenced this pull request Aug 29, 2018

expression: check max_allowed_packet constraint for function insert (p…

2a92e71

…ingcap#7502)

shenli pushed a commit that referenced this pull request Aug 29, 2018

expression: check max_allowed_packet constraint for function insert (#…

9d09f1d

…7502) (#7528)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression: check max_allowed_packet constraint for function insert #7502

expression: check max_allowed_packet constraint for function insert #7502

eurekaka commented Aug 27, 2018 •

edited

Loading

CLAassistant commented Aug 27, 2018 •

edited

Loading

eurekaka commented Aug 27, 2018

zz-jason Aug 27, 2018

zz-jason Aug 27, 2018

zz-jason commented Aug 27, 2018

zz-jason commented Aug 28, 2018

zz-jason commented Aug 28, 2018

zz-jason commented Aug 28, 2018

zz-jason commented Aug 28, 2018

alivxxx Aug 28, 2018

eurekaka Aug 28, 2018

eurekaka Aug 28, 2018

zz-jason Aug 28, 2018

alivxxx Aug 28, 2018

zz-jason Aug 28, 2018

zz-jason Aug 28, 2018 •

edited

Loading

eurekaka Aug 28, 2018

eurekaka commented Aug 28, 2018

eurekaka commented Aug 28, 2018

eurekaka commented Aug 29, 2018

iamxy commented Aug 29, 2018

iamxy commented Aug 29, 2018

eurekaka commented Aug 29, 2018

eurekaka commented Aug 29, 2018

zz-jason left a comment

alivxxx left a comment

expression: check max_allowed_packet constraint for function insert #7502

expression: check max_allowed_packet constraint for function insert #7502

Conversation

eurekaka commented Aug 27, 2018 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

CLAassistant commented Aug 27, 2018 • edited Loading

eurekaka commented Aug 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason commented Aug 27, 2018

zz-jason commented Aug 28, 2018

zz-jason commented Aug 28, 2018

zz-jason commented Aug 28, 2018

zz-jason commented Aug 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eurekaka commented Aug 28, 2018

eurekaka commented Aug 28, 2018

eurekaka commented Aug 29, 2018

iamxy commented Aug 29, 2018

iamxy commented Aug 29, 2018

eurekaka commented Aug 29, 2018

eurekaka commented Aug 29, 2018

zz-jason left a comment

Choose a reason for hiding this comment

alivxxx left a comment

Choose a reason for hiding this comment

eurekaka commented Aug 27, 2018 •

edited

Loading

CLAassistant commented Aug 27, 2018 •

edited

Loading

zz-jason Aug 28, 2018 •

edited

Loading