-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
statistics: refactor stats meta handling to use DeltaUpdate for multi-table support #58657
statistics: refactor stats meta handling to use DeltaUpdate for multi-table support #58657
Conversation
/retest |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #58657 +/- ##
================================================
+ Coverage 73.0885% 74.7839% +1.6953%
================================================
Files 1676 1707 +31
Lines 463646 482775 +19129
================================================
+ Hits 338872 361038 +22166
+ Misses 103927 99766 -4161
- Partials 20847 21971 +1124
Flags with carried forward coverage won't be shown. Click here to find out more.
|
/retest |
de24b35
to
19b12d5
Compare
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.
🔢 Self-check (PR reviewed by myself and ready for feedback.)
} | ||
|
||
// UpdateStatsMeta updates the stats meta for multiple tables. | ||
// It uses the INSERT INTO ... ON DUPLICATE KEY UPDATE syntax to fill the missing records. |
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.
I saw you update the function from one tabledelta to multi DeltaUpdate. But there is no code in PR to use the multi DeltaUpdate. Will/Where you use the multi interface in the future?
If the answer is yes, could you please add some test for the multi deltaupdate which is not same as the past.
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.
As I mentioned in the PR body, this PR split from #58331. You can find the usage from there.
If the answer is yes, could you please add some test for the multi deltaupdate which is not same as the past.
We have a lot of tests to check the count and modify_count. I think they have covered this change. To verify it independently, we need to use the mock session to execute the SQL. From my previous experience of using and maintaining it, it is difficult to understand and maintain. (Difficult to follow the mock logic and the mock framework.)
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.
I also verified the SQL change.
- First create the test table:
CREATE TABLE IF NOT EXISTS mysql.stats_meta (
version BIGINT UNSIGNED NOT NULL,
table_id BIGINT UNSIGNED NOT NULL,
modify_count BIGINT UNSIGNED NOT NULL DEFAULT 0,
count BIGINT UNSIGNED NOT NULL DEFAULT 0,
PRIMARY KEY (table_id)
) ENGINE=InnoDB;
- Insert initial test data:
INSERT INTO mysql.stats_meta (version, table_id, modify_count, count)
VALUES (900, 100, 10, 25)
ON DUPLICATE KEY UPDATE
version = VALUES(version),
modify_count = VALUES(modify_count),
count = VALUES(count);
- array version of negative value handling SQL:
INSERT INTO mysql.stats_meta (version, table_id, modify_count, count)
VALUES (1000, 100, 1, 10)
ON DUPLICATE KEY UPDATE
version = VALUES(version),
modify_count = modify_count + VALUES(modify_count),
count = IF(count > VALUES(count), count - VALUES(count), 0);
- or a single version of the negative value handling SQL:
INSERT INTO mysql.stats_meta (version, table_id, modify_count, count)
VALUES (1000, 100, 1, 0)
ON DUPLICATE KEY UPDATE
version = VALUES(version),
modify_count = modify_count + VALUES(modify_count),
count = IF(count > 10, count - 10, 0);
- View results:
SELECT * FROM mysql.stats_meta WHERE table_id = 100;
For other changes, it is easy to follow, only for the negitive number there is a little refactoring.
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.
So there is no good way to test multiple table updates now because we're not using it yet. I guess we can add some tests after we start using it.
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.
(Difficult to follow the mock logic and the mock framework.)
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.
I have no problem leaving the manual testing steps in the PR.
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.
As I mentioned in the PR body, this PR split from #58331. You can find the usage from there.
If the answer is yes, could you please add some test for the multi deltaupdate which is not same as the past.
We have a lot of tests to check the count and modify_count. I think they have covered this change. To verify it independently, we need to use the mock session to execute the SQL. From my previous experience of using and maintaining it, it is difficult to understand and maintain. (Difficult to follow the mock logic and the mock framework.)
OK, no question for me. Just want to know what is this PR used for.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qw4990, winoros The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test all |
…-table support Signed-off-by: Rustin170506 <[email protected]>
Signed-off-by: Rustin170506 <[email protected]>
Signed-off-by: Rustin170506 <[email protected]>
Signed-off-by: Rustin170506 <[email protected]>
19b12d5
to
4b3bc0e
Compare
Signed-off-by: Rustin170506 <[email protected]>
/retest |
…long-vector * commit '510d0037b18f258f505abc6cf13a8128563e9359': *: upgrade pd client to make sure tso client initiate successfully (#58752) ttl, test: scale TTL workers during the fault tests (#58750) planner: improve warning messages for unsupported HASH_JOIN hints (#58646) planner: prealloc the slices in the SplitCorColAccessCondFromFilters (#58785) ddl: supports non-unique global index (#58678) util/stmtsummary: add the network traffic related fields (#58101) var: enable `pd_enable_follower_handle_region` as default (#58385) statistics: refactor stats meta handling to use DeltaUpdate for multi-table support (#58657) parser: move 'model' to 'ast' pkg (#58704) statistics: add recover to protect background task (#58739) disttask: cancel subtask context if scheduled away (#58615) *: don't handle live updates of column size (#58596) *: fix a bug for default_authentication_plugin (2) (#58723) dupdetect: gRPC cancel should trigger retry (#58542) *: fix a bug for default_authentication_plugin (#57391) distsql: Fix backoff execution info inaccurate issue (#58707)
In response to a cherrypick label: new pull request created to branch |
What problem does this PR solve?
Issue Number: ref #57869
Problem Summary:
What changed and how does it work?
If you want to understand how it works, read more at #57869 (comment).
This PR split from #58331.
Just updated the
UpdateStatsMeta
can handle multiple detla table updates by taking a slice of DeltaUpdate as an argument.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.