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.

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: do not load unnecessary index statistics #54060

Merged

Conversation

Rustin170506
Copy link
Member

@Rustin170506 Rustin170506 commented Jun 17, 2024

What problem does this PR solve?

Issue Number: close #54022

Problem Summary:

See more at the issue.

A quick summary:
If you have tidb_opt_objective='determine' enabled, it will trigger some async stats load for some tables. If there tables histogram isn't in the system, it would cause some problems when we try to load it.

What changed and how does it work?

Check that the index statistics are really needed before loading them.

If we don't check it here, it will cause some problems if the index statistics don't exist.

Note: We still don't know why the histogram record for the index is missing. There are some possible issues:

  1. When users create this table, they may not have created this index yet, so we don't create the record for it automatically. (But if this is the case, I don't why do we have the index in the stats cache information)
  2. It may have been accidentally dropped, but I can't find any scenario where we would only drop one index for a table.

What happened in the test case:

  1. We create a table and the statsHandle.Update(do.InfoSchema()) will load this table into the stats cache. You can test it by breakpoint here:
    image
  2. We drop the stats of the stats, it will clean up all system table records for this table.
  3. We insert some data and wait for the modfiy_count and the count is not null in the mysql.stats_meta.
  4. Try to select some data from this table by ID, it would trigger an async load. Then we get that bug.
    https://github.com/hi-rustin/tidb/blob/648fc6386aa9aabb365baf0912f1e18c4ab74d95/pkg/planner/core/logical_plan_builder.go#L4824
  5. So in the test case we check if the index is still in the needed item array after we fix it.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test
  • No need to test
    • I checked and no code files have been changed.

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Fix a memory leak when loading index statistics.
修复加载 index 统计信息可能会造成内存泄漏的问题

@ti-chi-bot ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/cherry-pick-not-approved size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 17, 2024
Signed-off-by: hi-rustin <[email protected]>
@Rustin170506 Rustin170506 changed the title statistics: do not load unnecessary index statistics WIP: statistics: do not load unnecessary index statistics Jun 17, 2024
@ti-chi-bot ti-chi-bot bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 17, 2024
Copy link

codecov bot commented Jun 17, 2024

Codecov Report

Attention: Patch coverage is 18.75000% with 13 lines in your changes missing coverage. Please review.

Please upload report for BASE (release-7.5@1563cc5). Learn more about missing BASE report.

Additional details and impacted files
@@               Coverage Diff                @@
##             release-7.5     #54060   +/-   ##
================================================
  Coverage               ?   72.0278%           
================================================
  Files                  ?       1411           
  Lines                  ?     410758           
  Branches               ?          0           
================================================
  Hits                   ?     295860           
  Misses                 ?      94993           
  Partials               ?      19905           
Flag Coverage Δ
unit 72.0278% <18.7500%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 53.9913% <0.0000%> (?)
parser ∅ <0.0000%> (?)
br 53.4535% <0.0000%> (?)

Signed-off-by: hi-rustin <[email protected]>
@Rustin170506
Copy link
Member Author

Rustin170506 commented Jun 18, 2024

Tested locally:
After:

  1. Start the cluster with patch: tiup playground v7.5.1 --db.binpath /Volumes/t7/code/tidb/bin/tidb-server
  2. Create a table: create table if not exists t(a int, b int, index ia(a));
  3. Drop the stats: drop stats t;
  4. Insert some data: insert into t value(1,1), (2,2);
  5. Wait for a while until we can see the stats: show stats_meta
mysql> show stats_meta
    -> ;
+---------+------------+----------------+---------------------+--------------+-----------+
| Db_name | Table_name | Partition_name | Update_time         | Modify_count | Row_count |
+---------+------------+----------------+---------------------+--------------+-----------+
| test    | t          |                | 2024-06-18 12:07:12 |            0 |         0 |
+---------+------------+----------------+---------------------+--------------+-----------+
1 row in set (0.01 sec)
  1. Enable determinate: set tidb_opt_objective='determinate';
  2. Try select data: select * from t where a = 1 and b = 1;
mysql> select * from t where a = 1 and b = 1;
+------+------+
| a    | b    |
+------+------+
|    1 |    1 |
+------+------+
1 row in set (0.01 sec)
  1. Check logs:
[2024/06/18 12:09:47.346 +08:00] [WARN] [read.go:583] ["Although the index stats is not required to load, an attempt is still made to load it."] [table_id=102] [hist_id=1]

Before:

  1. Start the cluster with patch: tiup playground v7.5.1 --db.binpath /Volumes/t7/code/tidb/bin/tidb-server
  2. Create a table: create table if not exists t(a int, b int, index ia(a));
  3. Drop the stats: drop stats t;
  4. Insert some data: insert into t value(1,1), (2,2);
  5. Wait for a while until we can see the stats: show stats_meta
mysql> show stats_meta
    -> ;
+---------+------------+----------------+---------------------+--------------+-----------+
| Db_name | Table_name | Partition_name | Update_time         | Modify_count | Row_count |
+---------+------------+----------------+---------------------+--------------+-----------+
| test    | t          |                | 2024-06-18 12:07:12 |            0 |         0 |
+---------+------------+----------------+---------------------+--------------+-----------+
1 row in set (0.01 sec)
  1. Enable determinate: set tidb_opt_objective='determinate';
  2. Try select data: select * from t where a = 1 and b = 1;
mysql> select * from t where a = 1 and b = 1;
+------+------+
| a    | b    |
+------+------+
|    1 |    1 |
+------+------+
1 row in set (0.01 sec)
  1. Check logs:
[2024/06/18 12:15:01.729 +08:00] [ERROR] [read.go:603] ["fail to get stats version for this histogram"] [table_id=102] [hist_id=1]
[2024/06/18 12:15:04.728 +08:00] [ERROR] [read.go:603] ["fail to get stats version for this histogram"] [table_id=102] [hist_id=1]

@Rustin170506 Rustin170506 changed the title WIP: statistics: do not load unnecessary index statistics statistics: do not load unnecessary index statistics Jun 18, 2024
@ti-chi-bot ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 18, 2024
Signed-off-by: hi-rustin <[email protected]>
Copy link
Member Author

@Rustin170506 Rustin170506 left a 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.)

@hawkingrei hawkingrei self-requested a review June 18, 2024 04:43
@ti-chi-bot ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 18, 2024
Signed-off-by: hi-rustin <[email protected]>
Signed-off-by: hi-rustin <[email protected]>
Copy link
Member Author

@Rustin170506 Rustin170506 left a 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.)

Copy link
Contributor

@elsa0520 elsa0520 left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-chi-bot ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jun 18, 2024
@hawkingrei
Copy link
Member

Tested locally: After:

1. Start the cluster with patch: `tiup playground v7.5.1  --db.binpath /Volumes/t7/code/tidb/bin/tidb-server`

2. Create a table: `create table if not exists t(a int, b int, index ia(a));`

3. Drop the stats: `drop stats t;`

4. Insert some data: `insert into t value(1,1), (2,2);`

5. Wait for a while until we can see the stats: `show stats_meta`
mysql> show stats_meta
    -> ;
+---------+------------+----------------+---------------------+--------------+-----------+
| Db_name | Table_name | Partition_name | Update_time         | Modify_count | Row_count |
+---------+------------+----------------+---------------------+--------------+-----------+
| test    | t          |                | 2024-06-18 12:07:12 |            0 |         0 |
+---------+------------+----------------+---------------------+--------------+-----------+
1 row in set (0.01 sec)
6. Enable determinate: `set tidb_opt_objective='determinate';`

7. Try select data: `select * from t where a = 1 and b = 1;`
mysql> select * from t where a = 1 and b = 1;
+------+------+
| a    | b    |
+------+------+
|    1 |    1 |
+------+------+
1 row in set (0.01 sec)
8. Check logs:
[2024/06/18 12:09:47.346 +08:00] [WARN] [read.go:583] ["Although the index stats is not required to load, an attempt is still made to load it."] [table_id=102] [hist_id=1]

Before:

1. Start the cluster with patch: `tiup playground v7.5.1  --db.binpath /Volumes/t7/code/tidb/bin/tidb-server`

2. Create a table: `create table if not exists t(a int, b int, index ia(a));`

3. Drop the stats: `drop stats t;`

4. Insert some data: `insert into t value(1,1), (2,2);`

5. Wait for a while until we can see the stats: `show stats_meta`
mysql> show stats_meta
    -> ;
+---------+------------+----------------+---------------------+--------------+-----------+
| Db_name | Table_name | Partition_name | Update_time         | Modify_count | Row_count |
+---------+------------+----------------+---------------------+--------------+-----------+
| test    | t          |                | 2024-06-18 12:07:12 |            0 |         0 |
+---------+------------+----------------+---------------------+--------------+-----------+
1 row in set (0.01 sec)
6. Enable determinate: `set tidb_opt_objective='determinate';`

7. Try select data: `select * from t where a = 1 and b = 1;`
mysql> select * from t where a = 1 and b = 1;
+------+------+
| a    | b    |
+------+------+
|    1 |    1 |
+------+------+
1 row in set (0.01 sec)
8. Check logs:
[2024/06/18 12:15:01.729 +08:00] [ERROR] [read.go:603] ["fail to get stats version for this histogram"] [table_id=102] [hist_id=1]
[2024/06/18 12:15:04.728 +08:00] [ERROR] [read.go:603] ["fail to get stats version for this histogram"] [table_id=102] [hist_id=1]

You can add this desciption into Manual test.

Signed-off-by: hi-rustin <[email protected]>
Comment on lines 248 to 251
require.Eventually(t, func() bool {
rows := tk.MustQuery("show stats_meta").Rows()
return len(rows) > 0
}, 2*time.Minute, 5*time.Millisecond)
Copy link
Member

Choose a reason for hiding this comment

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

We can use this to trigger it immediately.

Suggested change
require.Eventually(t, func() bool {
rows := tk.MustQuery("show stats_meta").Rows()
return len(rows) > 0
}, 2*time.Minute, 5*time.Millisecond)
h := dom.StatsHandle()
require.NoError(t, h.DumpStatsDeltaToKV(true))
require.NoError(t, h.Update(dom.InfoSchema()))

Copy link
Member Author

@Rustin170506 Rustin170506 Jun 18, 2024

Choose a reason for hiding this comment

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

Nice! I forgot we can directly call an update here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

Signed-off-by: hi-rustin <[email protected]>
@ti-chi-bot ti-chi-bot added the cherry-pick-approved Cherry pick PR approved by release team. label Jun 18, 2024
Copy link

ti-chi-bot bot commented Jun 18, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elsa0520, time-and-fate

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added approved lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 18, 2024
Copy link

ti-chi-bot bot commented Jun 18, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-06-18 06:54:50.954675945 +0000 UTC m=+97817.440164778: ☑️ agreed by elsa0520.
  • 2024-06-18 08:23:05.483640997 +0000 UTC m=+103111.969129828: ☑️ agreed by time-and-fate.

@ti-chi-bot ti-chi-bot bot merged commit 3c599cd into pingcap:release-7.5 Jun 18, 2024
17 of 18 checks passed
@Rustin170506 Rustin170506 added the needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. label Jun 18, 2024
ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Jun 18, 2024
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-7.1: #54087.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved cherry-pick-approved Cherry pick PR approved by release team. lgtm needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants