expression: handle max_allowed_packet warnings for to_base64 function. #7266

supernan1994 · 2018-08-02T16:44:25Z

What have you changed? (mandatory)

Return NULL and a warning when the result of to_base64 exceeds max_allowed_packetbs.
Before this PR:

mysql> show variables like 'max_allowed_packet';
+--------------------+-------+
| Variable_name      | Value |
+--------------------+-------+
| max_allowed_packet | 3    |
+--------------------+-------+
1 row in set (0.01 sec)

mysql> select to_base64("abc");
+------------------+
| to_base64("abc") |
+------------------+
| YWJj             |
+------------------+
1 row in set (0.00 sec)

After this PR:

mysql> show variables like 'max_allowed_packet';
+--------------------+-------+
| Variable_name      | Value |
+--------------------+-------+
| max_allowed_packet | 3    |
+--------------------+-------+
1 row in set (0.01 sec)

mysql> select to_base64("abc");
+------------------+
| to_base64("abc") |
+------------------+
| NULL             |
+------------------+
1 row in set, 1 warning (0.00 sec)

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

This PR calculates length of to_base64 result by function base64NeededEncodedLength before executing core of to_base64 logic, and compares length with global variable max_allowed_packet.
unit tests are added for covering three circumstances:

result hasn't line feed
result has line feed, last few characters of result were filled with =
result has many line feed

What is the type of the changes? (mandatory)

Bug fix (non-breaking change which fixes an issue)

How has this PR been tested? (mandatory)

unit test
explain test
manual test

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

no

Does this PR affect tidb-ansible update? (mandatory)

no

Does this PR need to be added to the release notes? (mandatory)

Yes
Please note:

Return `NULL` when the result of function `TO_BASE64` is larger than `max_allowed_packet`

Refer to a related PR or issue link (optional)

to #7153

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

sre-bot · 2018-08-02T16:44:28Z

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

CLAassistant · 2018-08-02T16:44:33Z

All committers have signed the CLA.

zz-jason · 2018-08-03T03:20:59Z

expression/builtin_string.go

@@ -3044,6 +3055,10 @@ func (b *builtinToBase64Sig) evalString(row chunk.Row) (d string, isNull bool, e
 		return "", isNull, errors.Trace(err)
 	}

+	if b.tp.Flen*mysql.MaxBytesOfCharacter > int(b.maxAllowedPacket) {


It's not correct to use Flen to decide whether the result of ToBase64 exceeds max_allowed_packet, I think we can learn from MySQL:

String *Item_func_to_base64::val_str_ascii(String *str) { String *res = args[0]->val_str(str); bool too_long = false; uint64 length; if (!res || res->length() > (uint)base64_encode_max_arg_length() || (too_long = ((length = base64_needed_encoded_length((uint64)res->length())) > current_thd->variables.max_allowed_packet)) || tmp_value.alloc((uint)length)) { null_value = 1; // NULL input, too long input, or OOM. if (too_long) { push_warning_printf( current_thd, Sql_condition::SL_WARNING, ER_WARN_ALLOWED_PACKET_OVERFLOWED, ER_THD(current_thd, ER_WARN_ALLOWED_PACKET_OVERFLOWED), func_name(), current_thd->variables.max_allowed_packet); } return 0; } base64_encode(res->ptr(), (int)res->length(), (char *)tmp_value.ptr()); DBUG_ASSERT(length > 0); tmp_value.length((uint)length - 1); // Without trailing '\0' null_value = 0; return &tmp_value; }

The above code is taken from: https://github.com/mysql/mysql-server/blob/5.7/sql/item_strfunc.cc#L690

sorry, thanks, I will correct it

Hi, @supernan1994 any update?

Sorry!!! I had a bussiness trip and was back yesterday, I will take a look at it tonight and tomorrow night @zz-jason

😄 take your time, it's just a just a friendly Ping

…xceeds max_allowed_packet

zz-jason · 2018-08-10T14:31:01Z

expression/builtin_string_test.go

+		input := chunk.NewChunkWithCapacity(colTypes, 1)
+		input.AppendString(0, test.args)
+		res, isNull, err := toBase64.evalString(input.GetRow(0))
+		if test.getErr {


all the added test.getErr is false, maybe this check can be replaced with: c.Assert(err, IsNil)

zz-jason · 2018-08-10T14:32:47Z

expression/builtin_string_test.go

+		c.Assert(res, Equals, test.expect)
+	}
+	warnings := s.ctx.GetSessionVars().StmtCtx.GetWarnings()
+	c.Assert(len(warnings), Equals, warningCount)


warningCount is always zero, seems the test of max_allowed_packets is not working

supernan1994 · 2018-08-10T15:15:32Z

expression/builtin_string_test.go

+		c.Assert(err, IsNil)
+		if test.isNil {
+			c.Assert(isNull, IsTrue)
+			warningCount += 1


warningCount will add 1 when to_base64 result is nil

can we reset the warning appended to s.ctx.GetSessionVars().StmtCtx and check the exactly warning count and warning content in each test case?

supernan1994 · 2018-08-10T15:19:28Z

It seems travis ci failed because of temporary network problem

go get github.com/etcd-io/gofail
# cd .; git clone https://github.com/etcd-io/gofail /home/travis/gopath/src/github.com/etcd-io/gofail
Cloning into '/home/travis/gopath/src/github.com/etcd-io/gofail'...
fatal: unable to access 'https://github.com/etcd-io/gofail/': GnuTLS recv error (-9): A TLS packet with unexpected length was received.
package github.com/etcd-io/gofail: exit status 128
make: *** [gotest] Error 1

The command "make dev" exited with 2.

Done. Your build exited with 1.

zz-jason · 2018-08-10T15:25:57Z

@supernan1994 re-triggered

zz-jason

Reviewed 1 of 2 files at r2, 1 of 1 files at r4.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @supernan1994 and @zz-jason)

zz-jason · 2018-08-10T15:54:21Z

/run-all-tests

supernan1994 · 2018-08-10T16:17:45Z

/run-all-tests

supernan1994 · 2018-08-10T16:19:27Z

/run-all-tests

supernan1994 · 2018-08-12T05:25:24Z

/run-all-tests

zz-jason · 2018-08-14T04:59:51Z

@XuHuaiyu PTAL

crazycs520

LGTM.

zz-jason · 2018-08-17T11:30:30Z

@supernan1994 It would be nice if you can cherry-pick this PR to the release-2.0 branch 😄

supernan1994 · 2018-08-17T13:47:46Z

@zz-jason OK!

pingcap#7266) (cherry picked from commit 016006f)

sisterdong added 4 commits August 2, 2018 01:02

expression: handle max_allowed_packet warnings for to_base64 functions

5bb5cf7

Merge branch 'develop'

33a10e3

Merge branch 'master' of https://github.com/pingcap/tidb

ea06d28

Merge branch 'master' of https://github.com/pingcap/tidb

9e310ec

ciscoxll added the contribution This PR is from a community contributor. label Aug 2, 2018

zz-jason reviewed Aug 3, 2018

View reviewed changes

zz-jason added type/compatibility component/expression labels Aug 3, 2018

zz-jason mentioned this pull request Aug 8, 2018

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

Open

13 tasks

sisterdong added 4 commits August 9, 2018 21:32

Merge branch 'master' of https://github.com/pingcap/tidb

8c54930

Merge branch 'master' of https://github.com/pingcap/tidb

abc9329

use need encode length to decide whether the result of ToBase64 e…

9700a53

…xceeds max_allowed_packet

Merge branch 'master' of https://github.com/pingcap/tidb

1bbf7f4

zz-jason reviewed Aug 10, 2018

View reviewed changes

remove get err check

0467a1f

supernan1994 commented Aug 10, 2018

View reviewed changes

check the exactly warning count and warning content in each test case

7f158ac

zz-jason reviewed Aug 10, 2018

View reviewed changes

zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 10, 2018

supernan1994 added 2 commits August 11, 2018 12:15

Merge branch 'master' into master

67ea3e2

Merge branch 'master' into master

78c3716

Merge branch 'master' into master

2c6dd39

Merge branch 'master' into master

67984ea

zz-jason added the status/all tests passed label Aug 14, 2018

crazycs520 reviewed Aug 14, 2018

View reviewed changes

crazycs520 added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 14, 2018

supernan1994 and others added 2 commits August 14, 2018 18:10

Merge branch 'master' into master

60f73b5

Merge branch 'master' into master

055c2af

zz-jason approved these changes Aug 14, 2018

View reviewed changes

Merge branch 'master' into master

857951c

zz-jason merged commit 016006f into pingcap:master Aug 15, 2018

supernan1994 pushed a commit to supernan1994/tidb that referenced this pull request Aug 17, 2018

expression: handle max_allowed_packet warnings for to_base64 function. (

6a283d6

pingcap#7266) (cherry picked from commit 016006f)

supernan1994 mentioned this pull request Aug 17, 2018

expression: handle max_allowed_packet warnings for to_base64 function. #7427

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression: handle max_allowed_packet warnings for to_base64 function. #7266

expression: handle max_allowed_packet warnings for to_base64 function. #7266

supernan1994 commented Aug 2, 2018 •

edited

Loading

sre-bot commented Aug 2, 2018

CLAassistant commented Aug 2, 2018 •

edited

Loading

zz-jason Aug 3, 2018

supernan1994 Aug 3, 2018

zz-jason Aug 8, 2018

supernan1994 Aug 8, 2018 •

edited

Loading

zz-jason Aug 9, 2018

zz-jason Aug 10, 2018

zz-jason Aug 10, 2018

supernan1994 Aug 10, 2018

zz-jason Aug 10, 2018

supernan1994 Aug 10, 2018

zz-jason Aug 10, 2018

supernan1994 commented Aug 10, 2018

zz-jason commented Aug 10, 2018

zz-jason left a comment

zz-jason commented Aug 10, 2018

supernan1994 commented Aug 10, 2018

supernan1994 commented Aug 10, 2018

supernan1994 commented Aug 12, 2018

zz-jason commented Aug 14, 2018

crazycs520 left a comment

zz-jason commented Aug 17, 2018

supernan1994 commented Aug 17, 2018

expression: handle max_allowed_packet warnings for to_base64 function. #7266

expression: handle max_allowed_packet warnings for to_base64 function. #7266

Conversation

supernan1994 commented Aug 2, 2018 • edited Loading

What have you changed? (mandatory)

What is the type of the changes? (mandatory)

How has this PR been tested? (mandatory)

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

Does this PR affect tidb-ansible update? (mandatory)

Does this PR need to be added to the release notes? (mandatory)

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

sre-bot commented Aug 2, 2018

CLAassistant commented Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supernan1994 Aug 8, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supernan1994 commented Aug 10, 2018

zz-jason commented Aug 10, 2018

zz-jason left a comment

Choose a reason for hiding this comment

zz-jason commented Aug 10, 2018

supernan1994 commented Aug 10, 2018

supernan1994 commented Aug 10, 2018

supernan1994 commented Aug 12, 2018

zz-jason commented Aug 14, 2018

crazycs520 left a comment

Choose a reason for hiding this comment

zz-jason commented Aug 17, 2018

supernan1994 commented Aug 17, 2018

supernan1994 commented Aug 2, 2018 •

edited

Loading

CLAassistant commented Aug 2, 2018 •

edited

Loading

supernan1994 Aug 8, 2018 •

edited

Loading