Skip to content

executor: copy row data instead of refereeing chunk.Row in some window functions.#11678

Merged
SunRunAway merged 7 commits into
pingcap:masterfrom
SunRunAway:issue11614-windowfunc
Aug 22, 2019
Merged

executor: copy row data instead of refereeing chunk.Row in some window functions.#11678
SunRunAway merged 7 commits into
pingcap:masterfrom
SunRunAway:issue11614-windowfunc

Conversation

@SunRunAway
Copy link
Copy Markdown
Contributor

@SunRunAway SunRunAway commented Aug 8, 2019

What problem does this PR solve?

Fixes #11614, and fixes #11626 .
If a chunk.Chunk reuse into a window function executor, it may break the internal memory reference in function RANK or PERCENT_RANK.

  1. Father calls chk := NewChunk()
  2. Father calls windowExecutor.Next(chk)
  3. The windowExecutor put column results into chk while refereeing the chunk.Row into PartialResult of function RANK or PERCENT_RANK.
  4. Father calls windowExecutor.Next(chk) again to collect next batch of result and reuses the same chk.
  5. chunk.Row refereced by PartialResult is broken when chk.Reset is called.
  • TODO: fix the other window functions which may lead this problem.

What is changed and how it works?

Change step 3 above, copy row data instead of refereeing chunk.Row in window functions which rely on data comparing.

Check List

Tests

  • Unit test

Code changes

  • None

Side effects

  • Possible performance regression
  • note: no func lag because it cannot be finished with old code.
name                                                           old time/op    new time/op    delta
WindowRows/(func:row_number,_ndv:10,_rows:1000)-12               65.7µs ± 9%    68.5µs ± 8%       ~     (p=0.523 n=3+3)
WindowRows/(func:row_number,_ndv:1000,_rows:1000)-12              131µs ± 2%     161µs ± 8%    +22.55%  (p=0.041 n=3+3)
WindowRows/(func:row_number,_ndv:10,_rows:100000)-12             5.98ms ± 1%    5.97ms ± 1%       ~     (p=0.784 n=3+3)
WindowRows/(func:row_number,_ndv:1000,_rows:100000)-12           6.19ms ± 2%    6.07ms ± 0%       ~     (p=0.173 n=3+3)
WindowFunctions/(func:row_number,_ndv:1000,_rows:100000)-12      6.07ms ± 2%    6.06ms ± 3%       ~     (p=0.942 n=3+3)
WindowFunctions/(func:rank,_ndv:1000,_rows:100000)-12            6.79ms ± 0%    9.61ms ± 1%    +41.60%  (p=0.000 n=3+3)
WindowFunctions/(func:dense_rank,_ndv:1000,_rows:100000)-12      6.81ms ± 1%    9.70ms ± 1%    +42.59%  (p=0.000 n=3+3)
WindowFunctions/(func:cume_dist,_ndv:1000,_rows:100000)-12       6.82ms ± 3%   10.09ms ± 1%    +47.99%  (p=0.000 n=3+3)
WindowFunctions/(func:percent_rank,_ndv:1000,_rows:100000)-12    6.86ms ± 1%    9.62ms ± 0%    +40.25%  (p=0.000 n=3+3)
WindowFunctions/(func:ntile,_ndv:1000,_rows:100000)-12           6.31ms ± 1%    6.17ms ± 1%       ~     (p=0.050 n=3+3)
WindowFunctions/(func:lead,_ndv:1000,_rows:100000)-12            8.04ms ± 1%   12.30ms ± 0%    +52.91%  (p=0.000 n=3+3)
WindowFunctions/(func:first_value,_ndv:1000,_rows:100000)-12     6.28ms ± 1%    6.44ms ± 0%       ~     (p=0.063 n=3+3)
WindowFunctions/(func:last_value,_ndv:1000,_rows:100000)-12      6.36ms ± 0%    6.40ms ± 1%       ~     (p=0.415 n=3+3)
WindowFunctions/(func:nth_value,_ndv:1000,_rows:100000)-12       6.48ms ± 1%    6.47ms ± 1%       ~     (p=0.871 n=3+3)

name                                                           old alloc/op   new alloc/op   delta
WindowRows/(func:row_number,_ndv:10,_rows:1000)-12               44.7kB ± 0%    44.7kB ± 0%       ~     (zero variance)
WindowRows/(func:row_number,_ndv:1000,_rows:1000)-12             81.0kB ± 1%    80.5kB ± 1%       ~     (p=0.523 n=3+3)
WindowRows/(func:row_number,_ndv:10,_rows:100000)-12              263kB ± 0%     263kB ± 0%       ~     (p=0.443 n=3+3)
WindowRows/(func:row_number,_ndv:1000,_rows:100000)-12            301kB ± 0%     299kB ± 1%       ~     (p=0.423 n=3+3)
WindowFunctions/(func:row_number,_ndv:1000,_rows:100000)-12       301kB ± 0%     301kB ± 0%       ~     (zero variance)
WindowFunctions/(func:rank,_ndv:1000,_rows:100000)-12             307kB ± 0%    6917kB ± 0%  +2155.27%  (p=0.000 n=3+3)
WindowFunctions/(func:dense_rank,_ndv:1000,_rows:100000)-12       306kB ± 0%    6921kB ± 0%  +2160.67%  (p=0.000 n=3+3)
WindowFunctions/(func:cume_dist,_ndv:1000,_rows:100000)-12        306kB ± 0%    6923kB ± 0%  +2161.01%  (p=0.000 n=3+3)
WindowFunctions/(func:percent_rank,_ndv:1000,_rows:100000)-12     306kB ± 0%    6929kB ± 0%  +2162.99%  (p=0.000 n=3+3)
WindowFunctions/(func:ntile,_ndv:1000,_rows:100000)-12            307kB ± 0%     301kB ± 0%     -1.95%  (p=0.000 n=3+3)
WindowFunctions/(func:lead,_ndv:1000,_rows:100000)-12             306kB ± 0%    5984kB ± 0%  +1857.22%  (p=0.000 n=3+3)
WindowFunctions/(func:first_value,_ndv:1000,_rows:100000)-12      301kB ± 0%     301kB ± 0%       ~     (zero variance)
WindowFunctions/(func:last_value,_ndv:1000,_rows:100000)-12       301kB ± 0%     301kB ± 0%       ~     (zero variance)
WindowFunctions/(func:nth_value,_ndv:1000,_rows:100000)-12        301kB ± 0%     301kB ± 0%       ~     (zero variance)

name                                                           old allocs/op  new allocs/op  delta
WindowRows/(func:row_number,_ndv:10,_rows:1000)-12                 74.0 ± 0%      74.0 ± 0%       ~     (zero variance)
WindowRows/(func:row_number,_ndv:1000,_rows:1000)-12                698 ± 3%       690 ± 1%       ~     (p=0.523 n=3+3)
WindowRows/(func:row_number,_ndv:10,_rows:100000)-12              1.94k ± 0%     1.94k ± 0%       ~     (p=1.000 n=3+3)
WindowRows/(func:row_number,_ndv:1000,_rows:100000)-12            3.00k ± 0%     3.00k ± 0%       ~     (p=0.423 n=3+3)
WindowFunctions/(func:row_number,_ndv:1000,_rows:100000)-12       3.00k ± 0%     3.00k ± 0%       ~     (zero variance)
WindowFunctions/(func:rank,_ndv:1000,_rows:100000)-12             3.01k ± 0%    23.81k ± 0%   +691.91%  (p=0.000 n=3+3)
WindowFunctions/(func:dense_rank,_ndv:1000,_rows:100000)-12       3.01k ± 0%    23.83k ± 0%   +692.33%  (p=0.000 n=3+3)
WindowFunctions/(func:cume_dist,_ndv:1000,_rows:100000)-12        3.01k ± 0%    23.83k ± 0%   +692.54%  (p=0.000 n=3+3)
WindowFunctions/(func:percent_rank,_ndv:1000,_rows:100000)-12     3.01k ± 0%    23.85k ± 0%   +693.17%  (p=0.000 n=3+3)
WindowFunctions/(func:ntile,_ndv:1000,_rows:100000)-12            3.01k ± 0%     3.00k ± 0%       ~     (zero variance)
WindowFunctions/(func:lead,_ndv:1000,_rows:100000)-12             3.01k ± 0%   111.01k ± 0%  +3591.58%  (p=0.000 n=3+3)
WindowFunctions/(func:first_value,_ndv:1000,_rows:100000)-12      3.00k ± 0%     3.00k ± 0%       ~     (zero variance)
WindowFunctions/(func:last_value,_ndv:1000,_rows:100000)-12       3.00k ± 0%     3.00k ± 0%       ~     (zero variance)
WindowFunctions/(func:nth_value,_ndv:1000,_rows:100000)-12        3.00k ± 0%     3.00k ± 0%       ~     (zero variance)

Related changes

  • Need to cherry-pick to the release branch

@SunRunAway
Copy link
Copy Markdown
Contributor Author

SunRunAway commented Aug 8, 2019

The unit test fails because it relies on #11637
Please ignore it and merge this pr firstly, then merge #11637

@foreyes
Copy link
Copy Markdown
Contributor

foreyes commented Aug 8, 2019

Did you reference a wrong PR number? @SunRunAway

@SunRunAway
Copy link
Copy Markdown
Contributor Author

@foreyes sorry, fixed.

Comment thread executor/aggfuncs/func_cume_dist.go Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 8, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@6f76bbe). Click here to learn what that means.
The diff coverage is 93.3014%.

@@             Coverage Diff             @@
##             master     #11678   +/-   ##
===========================================
  Coverage          ?   81.4533%           
===========================================
  Files             ?        432           
  Lines             ?      93192           
  Branches          ?          0           
===========================================
  Hits              ?      75908           
  Misses            ?      11866           
  Partials          ?       5418

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Aug 8, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Copy Markdown
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

LGTM

@alivxxx alivxxx added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 8, 2019
Comment thread executor/aggfuncs/func_cume_dist.go Outdated
Comment thread executor/aggfuncs/func_rank.go Outdated
@SunRunAway SunRunAway force-pushed the issue11614-windowfunc branch from 71e9ca9 to c4c83a0 Compare August 13, 2019 03:47
@lzmhhh123 lzmhhh123 removed their request for review August 13, 2019 08:33
@SunRunAway SunRunAway force-pushed the issue11614-windowfunc branch 6 times, most recently from 2410a7a to 0c430ed Compare August 13, 2019 13:00
Comment thread executor/aggfuncs/func_rank.go Outdated
Comment thread executor/aggfuncs/func_rank.go Outdated
Comment thread executor/aggfuncs/func_value.go Outdated
Comment thread executor/aggfuncs/func_value.go Outdated
Comment thread executor/aggfuncs/func_value.go Outdated
Comment thread executor/aggfuncs/func_value.go Outdated
Comment thread session/tidb.go
Comment thread types/mydecimal.go
@SunRunAway SunRunAway requested a review from zz-jason August 16, 2019 09:25
Comment thread executor/aggfuncs/func_lead_lag.go
Comment thread executor/aggfuncs/func_lead_lag.go
Copy link
Copy Markdown
Member

@zz-jason zz-jason 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 the status/LGT1 Indicates that a PR has LGTM 1. label Aug 19, 2019
@zz-jason zz-jason requested a review from alivxxx August 19, 2019 09:10
Copy link
Copy Markdown
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

LGTM

@SunRunAway SunRunAway merged commit 8d165f0 into pingcap:master Aug 22, 2019
@SunRunAway SunRunAway deleted the issue11614-windowfunc branch August 22, 2019 02:58
@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Aug 22, 2019

cherry pick to release-3.0 in PR #11823

SunRunAway added a commit to SunRunAway/tidb that referenced this pull request Oct 12, 2019
sre-bot pushed a commit that referenced this pull request Oct 16, 2019
sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Oct 16, 2019
sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Oct 16, 2019
XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sig/execution SIG execution status/LGT1 Indicates that a PR has LGTM 1. type/bugfix This PR fixes a bug.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

using window function "first_value" get "index out of range" runtime error window function rank() sometimes lead to tidb-server panic

6 participants