statistics: remove lockedTable cache #46555

Rustin170506 · 2023-08-31T06:41:15Z

What problem does this PR solve?

Issue Number: ref #46351

Problem Summary:

What changed and how it works?

Removed the lockedTable cache. The reason we're removing it is because it doesn't help. It's locked and unlocked so infrequently that it doesn't matter if we query it every time. This way we don't need to load it as often.
Removed LoadLockedTables background task
Use the read lock to protect the SQLExecutor
Move all code to lockstats pkg

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

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

Documentation

Release note

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

None

codecov · 2023-08-31T06:52:29Z

Codecov Report

Merging #46555 (a0276db) into master (80da849) will decrease coverage by 0.8824%.
Report is 10 commits behind head on master.
The diff coverage is 72.1925%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #46555        +/-   ##
================================================
- Coverage   73.3411%   72.4588%   -0.8824%     
================================================
  Files          1309       1331        +22     
  Lines        394560     403346      +8786     
================================================
+ Hits         289375     292260      +2885     
- Misses        86769      92556      +5787     
- Partials      18416      18530       +114

Flag	Coverage Δ
integration	`25.6415% <19.7861%> (?)`
unit	`73.3467% <72.1925%> (+0.0055%)`	⬆️

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

Components	Coverage Δ
dumpling	`54.0444% <ø> (ø)`
parser	`84.9628% <ø> (-0.0145%)`	⬇️
br	`47.4689% <ø> (-4.8818%)`	⬇️

Rustin170506 · 2023-08-31T07:29:55Z

Start the TiDB cluster: tiup playground nightly --tiflash 0 --db.binpath /Users/hi-rustin/GolandProjects/tidb/bin/tidb-server
Create table and analyze it:

create table t(a int, b int);
insert into t values (1,2), (3,4), (5,6), (7,8);
analyze table t;
show warnings;

Lock table: lock stats t;
Analyze it again: analyze table t;
Check the warnings: show warnings;

mysql> show warnings;
+---------+------+-----------------------------------------------------------------------------------------------------------------------------------------+
| Level   | Code | Message                                                                                                                                 |
+---------+------+-----------------------------------------------------------------------------------------------------------------------------------------+
| Note    | 1105 | Analyze use auto adjusted sample rate 1.000000 for table test.t, reason to use this rate is "use min(1, 110000/4) as the sample-rate=1" |
| Warning | 1105 | skip analyze locked table: t                                                                                                            |
+---------+------+-----------------------------------------------------------------------------------------------------------------------------------------+

Check the mysql.stats_table_locked table:

mysql> use mysql;
Reading table information for completion of table and column names
You can turn off this feature to get a quicker startup with -A

Database changed
mysql> select * from stats_table_locked;
+----------+--------------+-------+--------------------+
| table_id | modify_count | count | version            |
+----------+--------------+-------+--------------------+
|       98 |            4 |     4 | 443863948666601483 |
+----------+--------------+-------+--------------------+
1 row in set (0.00 sec)

Try to lock it again: lock stats t;

mysql> use test;
Reading table information for completion of table and column names
You can turn off this feature to get a quicker startup with -A

Database changed
mysql> lock stats t;
Query OK, 0 rows affected, 1 warning (0.01 sec)

mysql> show warnings;
+---------+------+-----------------------------------+
| Level   | Code | Message                           |
+---------+------+-----------------------------------+
| Warning | 1105 | skip locking locked table: test.t |
+---------+------+-----------------------------------+
1 row in set (0.00 sec)

Show locked stats: show stats_locked;.

mysql> show stats_locked;
+---------+------------+----------------+--------+
| Db_name | Table_name | Partition_name | Status |
+---------+------------+----------------+--------+
| test    | t          |                | locked |
+---------+------------+----------------+--------+
1 row in set (0.00 sec)

Unlock the table: unlock stats t;
Check the mysql.stats_table_locked table again:

mysql> select * from stats_table_locked;
Empty set (0.00 sec)

Show locked stats: show stats_locked;

mysql> show stats_locked;
Empty set (0.00 sec)

Unlock the table again and check warnings: unlock stats t;

mysql> unlock stats t;
Query OK, 0 rows affected, 1 warning (0.00 sec)

mysql> show warnings;
+---------+------+---------------------------------------+
| Level   | Code | Message                               |
+---------+------+---------------------------------------+
| Warning | 1105 | skip unlocking unlocked table: test.t |
+---------+------+---------------------------------------+
1 row in set (0.00 sec)

qw4990 · 2023-08-31T10:27:07Z

statistics/handle/lock_stats_handler.go

 }

 // IsTableLocked check whether table is locked in handle with Handle.Mutex
-func (h *Handle) IsTableLocked(tableID int64) bool {
+func (h *Handle) IsTableLocked(tableID int64) (bool, error) {


Can we allow this method to accept multiple table IDs like IsTableLocked(tableIDs ...int64) ([]bool, error) to get a better performance? Otherwise when executing show stats locked and there a N tables, we have to execute SELECT table_id FROM mysql.stats_table_locked N times, which is a little slow.

Good point. I tried it this morning. It makes the code complicated. I will try it again.

Addressed in a6a0bf3 (#46555)

statistics/handle/lock_stats_handler.go

qw4990 · 2023-09-01T06:26:07Z

statistics/handle/update.go

+		checkedTableIDs, err := h.QueryTablesLockedStatuses(tids...)
+		if err != nil {
+			logutil.BgLogger().Error("check table lock failed",
+				zap.String("category", "stats"), zap.Error(err))
+			continue
+		}
+
 		for _, tbl := range tbls {
-			//if table locked, skip analyze
-			if h.IsTableLocked(tbl.Meta().ID) {
+			// If table locked, skip analyze.
+			if checkedTableIDs[tbl.Meta().ID] {
+				logutil.BgLogger().Info("skip analyze locked table", zap.String("category", "stats"),
+					zap.String("db", db), zap.String("table", tbl.Meta().Name.O))


Please add some comments here to notice this check is not complete. For example, some other TiDB node may lock or unlock this table between this check and the analyze command.

Better to enhance or remove this logic in the future.

statistics: rename statistics: rename statistics: rename statistics: rename

ti-chi-bot · 2023-09-01T07:48:18Z

[LGTM Timeline notifier]

Timeline:

2023-09-01 06:29:09.957485443 +0000 UTC m=+2081314.506501415: ☑️ agreed by qw4990.
2023-09-01 07:48:16.953042852 +0000 UTC m=+2086061.502058823: ☑️ agreed by hawkingrei.

Rustin170506 · 2023-09-01T07:49:37Z

/retest

Rustin170506 · 2023-09-01T08:25:57Z

/retest

time-and-fate

Why are we switching from Lock to RLock? Would this possibly cause problems when there are concurrent operations?

Rustin170506 · 2023-09-01T10:06:06Z

Why are we switching from Lock to RLock? Would this possibly cause problems when there are concurrent operations?

It was a mistake. I reverted it.

time-and-fate

A typo in filename statistics/handle/handletest/statslock/stats_lcok_test.go. Seems it has been there for a long time.
Personally, I think it'll be better if we extract something like query_lock.go from lock_stats.go. Feel free to update it or keep it unchanged.

time-and-fate · 2023-09-01T10:29:55Z

executor/analyze.go

+		} else {
+			tbl, ok := infoSchema.TableByID(tid)
+			if !ok {
+				// Ignore this table because it may have been dropped.


This line can be removed because dropping this table also wouldn't make this case happen.

ti-chi-bot · 2023-09-01T10:32:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hawkingrei, qw4990, 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:

~~OWNERS~~ [hawkingrei,qw4990,time-and-fate]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Rustin170506 · 2023-09-01T11:17:10Z

/retest

Rustin170506 · 2023-09-01T14:44:37Z

/retest

Rustin170506 · 2023-09-01T16:53:27Z

/retest

Rustin170506 · 2023-09-02T00:49:39Z

/retest

hawkingrei · 2023-09-02T03:31:41Z

/test all

Rustin170506 · 2023-09-02T03:55:36Z

A typo in filename statistics/handle/handletest/statslock/stats_lcok_test.go. Seems it has been there for a long time.

Personally, I think it'll be better if we extract something like query_lock.go from lock_stats.go. Feel free to update it or keep it unchanged.

I will deal with them in my next PR.
Thanks for your review! 💚 💙 💜 💛

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 31, 2023

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 31, 2023

Rustin170506 requested review from qw4990 and time-and-fate August 31, 2023 08:30

Rustin170506 added sig/planner SIG: Planner component/statistics labels Aug 31, 2023

qw4990 reviewed Aug 31, 2023

View reviewed changes

statistics/handle/lock_stats_handler.go Outdated Show resolved Hide resolved

Rustin170506 force-pushed the rustin-patch-unlock-cache branch 3 times, most recently from fe795da to a6a0bf3 Compare September 1, 2023 06:01

Rustin170506 requested a review from qw4990 September 1, 2023 06:06

qw4990 reviewed Sep 1, 2023

View reviewed changes

qw4990 approved these changes Sep 1, 2023

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 1, 2023

Rustin170506 added 6 commits September 1, 2023 14:46

statistics: remove lockedTable cache

fe02ad6

statistics: revert rename

8d34e90

statistics: fix lint

792c446

statistics: move to lockstats package

0b97019

statistics: move to lockstats package

6a515b5

statistics: remove dead code

6ba8d34

Rustin170506 added 3 commits September 1, 2023 14:46

statistics: add comment

5741340

statistics: batch query

c25decd

statistics: rename statistics: rename statistics: rename statistics: rename

statistics: fix lint

9218d19

Rustin170506 force-pushed the rustin-patch-unlock-cache branch from 5c971df to 9218d19 Compare September 1, 2023 06:46

Rustin170506 added 2 commits September 1, 2023 15:03

statistics: fix test

e932e3a

statistics: add comments

b34abd7

hawkingrei approved these changes Sep 1, 2023

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 1, 2023

time-and-fate reviewed Sep 1, 2023

View reviewed changes

statistics: use write lock

a0276db

Rustin170506 requested a review from time-and-fate September 1, 2023 10:24

time-and-fate approved these changes Sep 1, 2023

View reviewed changes

ti-chi-bot bot merged commit 93dce26 into pingcap:master Sep 2, 2023
4 of 9 checks passed

Rustin170506 mentioned this pull request Sep 4, 2023

statistics: use session pool when locking or unlocking table stats #46611

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

statistics: remove lockedTable cache #46555

statistics: remove lockedTable cache #46555

Rustin170506 commented Aug 31, 2023 •

edited

Loading

codecov bot commented Aug 31, 2023 •

edited

Loading

Rustin170506 commented Aug 31, 2023

qw4990 Aug 31, 2023 •

edited

Loading

Rustin170506 Aug 31, 2023

Rustin170506 Sep 1, 2023

qw4990 Sep 1, 2023

qw4990 Sep 1, 2023

ti-chi-bot bot commented Sep 1, 2023

Rustin170506 commented Sep 1, 2023

Rustin170506 commented Sep 1, 2023

time-and-fate left a comment •

edited

Loading

Rustin170506 commented Sep 1, 2023

time-and-fate left a comment •

edited

Loading

time-and-fate Sep 1, 2023

ti-chi-bot bot commented Sep 1, 2023

Rustin170506 commented Sep 1, 2023

Rustin170506 commented Sep 1, 2023

Rustin170506 commented Sep 1, 2023

Rustin170506 commented Sep 2, 2023

hawkingrei commented Sep 2, 2023

Rustin170506 commented Sep 2, 2023

statistics: remove lockedTable cache #46555

statistics: remove lockedTable cache #46555

Conversation

Rustin170506 commented Aug 31, 2023 • edited Loading

What problem does this PR solve?

What changed and how it works?

Check List

Release note

codecov bot commented Aug 31, 2023 • edited Loading

Codecov Report

Rustin170506 commented Aug 31, 2023

qw4990 Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Rustin170506 Aug 31, 2023

Choose a reason for hiding this comment

Rustin170506 Sep 1, 2023

Choose a reason for hiding this comment

qw4990 Sep 1, 2023

Choose a reason for hiding this comment

qw4990 Sep 1, 2023

Choose a reason for hiding this comment

ti-chi-bot bot commented Sep 1, 2023

[LGTM Timeline notifier]

Rustin170506 commented Sep 1, 2023

Rustin170506 commented Sep 1, 2023

time-and-fate left a comment • edited Loading

Choose a reason for hiding this comment

Rustin170506 commented Sep 1, 2023

time-and-fate left a comment • edited Loading

Choose a reason for hiding this comment

time-and-fate Sep 1, 2023

Choose a reason for hiding this comment

ti-chi-bot bot commented Sep 1, 2023

Rustin170506 commented Sep 1, 2023

Rustin170506 commented Sep 1, 2023

Rustin170506 commented Sep 1, 2023

Rustin170506 commented Sep 2, 2023

hawkingrei commented Sep 2, 2023

Rustin170506 commented Sep 2, 2023

Rustin170506 commented Aug 31, 2023 •

edited

Loading

codecov bot commented Aug 31, 2023 •

edited

Loading

qw4990 Aug 31, 2023 •

edited

Loading

time-and-fate left a comment •

edited

Loading

time-and-fate left a comment •

edited

Loading