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.

Sign up for GitHub

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

Jump to bottom

expression: handle max_allowed_packet warnings for pad functions #7171

Merged
merged 10 commits into from
Jul 31, 2018
Merged

expression: handle max_allowed_packet warnings for pad functions #7171

merged 10 commits into from
Jul 31, 2018

Conversation

zz-jason
Copy link
Member

What have you changed? (mandatory)

Return NULL and a warning when the result of lpad or rpad exceeds max_allowed_packetbs.

Before this PR, select rpad('hello', 18446744073709551617, '1'); would make tidb-server panic by runtime error: makeslice: len out of range.

After this PR:

TiDB(localhost:4000) > select rpad('hello', 18446744073709551617, '1');
+------------------------------------------+
| rpad('hello', 18446744073709551617, '1') |
+------------------------------------------+
| NULL                                     |
+------------------------------------------+
1 row in set, 2 warnings (0.00 sec)

TiDB(localhost:4000) > show warnings;
+---------+------+----------------------------------------------------------------------------+
| Level   | Code | Message                                                                    |
+---------+------+----------------------------------------------------------------------------+
| Warning | 1292 | Truncated incorrect DECIMAL value: '18446744073709551617'                  |
| Warning | 1301 | Result of rpad() was larger than max_allowed_packet (67108864) - truncated |
+---------+------+----------------------------------------------------------------------------+
2 rows in set (0.00 sec)

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

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

release note:

Return `NULL` when the result of function `LPAD`/`RPAD` 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)

@zz-jason zz-jason mentioned this pull request Jul 27, 2018
Open
13 tasks
@zz-jason zz-jason added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 27, 2018
@zz-jason
Copy link
Member Author

/run-all-tests

@zz-jason
Copy link
Member Author

@winoros @XuHuaiyu PTAL

@zz-jason zz-jason mentioned this pull request Jul 30, 2018
Merged
XuHuaiyu
XuHuaiyu reviewed Jul 30, 2018
View reviewed changes
expression/builtin_string.go Outdated
@@ -1760,24 +1760,33 @@ func (c *lpadFunctionClass) getFunction(ctx sessionctx.Context, args []Expressio
bf.tp.Flen = getFlen4LpadAndRpad(bf.ctx, args[1])
SetBinFlagOrBinStr(args[0].GetType(), bf.tp)
SetBinFlagOrBinStr(args[2].GetType(), bf.tp)

valStr, _ := ctx.GetSessionVars().GetSystemVar("max_allowed_packet")
Copy link
Contributor

Choose a reason for hiding this comment

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

make "max_allowed_packet" a constant in sessionctx/variable/sysvar.go?

winoros
winoros reviewed Jul 30, 2018
View reviewed changes
mysql/errname.go
@@ -316,7 +316,7 @@ var MySQLErrName = map[uint16]string{
ErrUnknownTimeZone: "Unknown or incorrect time zone: '%-.64s'",
ErrWarnInvalidTimestamp: "Invalid TIMESTAMP value in column '%s' at row %d",
ErrInvalidCharacterString: "Invalid %s character string: '%.64s'",
ErrWarnAllowedPacketOverflowed: "Result of %s() was larger than maxAllowedPacket (%d) - truncated",
ErrWarnAllowedPacketOverflowed: "Result of %s() was larger than max_allowed_packet (%d) - truncated",
Copy link
Member

Choose a reason for hiding this comment

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

This is original warning msg in mysql?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this error message comes from the mysql-server code base.

@zz-jason
Copy link
Member Author

@XuHuaiyu Done, PTAL

XuHuaiyu
XuHuaiyu approved these changes Jul 31, 2018
View reviewed changes
Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason
Copy link
Member Author

/run-all-tests

@zz-jason zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 31, 2018
jackysp
jackysp approved these changes Jul 31, 2018
View reviewed changes
Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 31, 2018
@zz-jason zz-jason merged commit c38f567 into pingcap:master Jul 31, 2018
@zz-jason zz-jason deleted the dev/maxAllowedPacket branch July 31, 2018 07:12
zz-jason added a commit to zz-jason/tidb that referenced this pull request Aug 2, 2018
@zz-jason
expression: handle max_allowed_packet warnings for pad functions (pin…
bfb5632 
…gcap#7171)
@zz-jason zz-jason mentioned this pull request Aug 2, 2018
Merged
zz-jason added a commit that referenced this pull request Aug 2, 2018
@zz-jason
expression: handle max_allowed_packet warnings for pad functions (#7171…
13d711f 
…) (#7244)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@winoros winoros winoros left review comments

@XuHuaiyu XuHuaiyu XuHuaiyu approved these changes

@jackysp jackysp jackysp approved these changes

Assignees
No one assigned
Labels
component/expression release-note Denotes a PR that will be considered when it comes time to generate release notes. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zz-jason @jackysp @winoros @XuHuaiyu