Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
topsql: fix issue of topsql failed catch the running SQL when topsql is enabled in execution #33861
topsql: fix issue of topsql failed catch the running SQL when topsql is enabled in execution #33861
Changes from 18 commits
a63c65b
f216da0
7405de7
11f8be3
f4dedc8
33e2f29
67fbc8f
0137849
f7bc22b
1112d3c
e05043b
8a7cdfa
ed1c56a
3fd11c6
c1e41bc
f406d0e
70c2f3f
24bd016
57a6506
3e17956
e448431
6483077
ee992f7
37e1a52
3b68826
b6c307b
0676fb9
4dfa429
3c2152a
ab7b3d3
4981f8c
663eb93
772c24f
c01ae67
24e7c33
1d43b59
2002f46
4af1e45
0e64ae3
de71fd4
a7f41bc
334b85e
22bcad1
48e980b
98dd8c3
fb64e1d
1b94423
eb773bc
da2673e
b965eb4
095f4df
43c5da3
cc5f652
0dd6dab
3da532e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note (no need to change in this PR): a better API design should be a hook framework for statement begin and statement finish events, instead of something xxxForTopSQL. (The observer pattern)
cc @mornyx @zhongzc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OnExecutionBegin
will be skipped executed after this return?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, since it has a performance impact when TopSQL is disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there are potential performance improvements here @mornyx @zhongzc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my sysbench point-get test,
OnExecutionBegin
andOnExecutionFinished
have around %3 QPS impact.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the previous implementation:
Note
if stats := a.Ctx.GetStmtStats(); stats != nil && topsqlstate.TopSQLEnabled()
. This means that the overhead ofOnExecutionBegin()
does not exist before when TopSQL is not enabled.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do a test, I think it will work. Since the session is serial, the only scenario where lock contention occurs is the collection of background goroutine. So in theory, segmented locks can effectively reduce the possibility of sessions being blocked by background goroutine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not from lock. I tried to remove all locks previously and found the performance impact was not reduced much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit strange, the logic of
OnExecutionBegin
is very simple (get an item from the map, then increment it). This shouldn't cause a 3% QPS regression... cc @crazycs520There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map-assign and map-access have become the main cause. A more complicated lock implementation will introduce extra overhead and worse performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why would a single map-assign and access cause this kind of drop? It doesn't make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it happen that
IsSQLAndPlanRegistered == true
whileIsSQLRegistered == false
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will happen, but it doesn't matter, the code can make sure if
IsSQLAndPlanRegistered
is true, then theSQL
andplan
must be registerd.