statistics: GCStats should not wrongly remove record of an existing table #58108

winoros · 2024-12-09T17:27:59Z

What problem does this PR solve?

Issue Number: close #57983

Problem Summary:

When we try to delete the stats. We need to first ensure that the table is already deleted.

What changed and how does it work?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
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

Release note

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

修复统计信息垃圾回收时可能错误的删除统计信息记录的问题
Fix the issue that the stat' GC process may wrongly delete records of statistics.

…able

codecov · 2024-12-09T17:45:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.9148%. Comparing base (2a1f646) to head (4136245).
Report is 196 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #58108        +/-   ##
================================================
+ Coverage   73.2116%   73.9148%   +0.7032%     
================================================
  Files          1675       1706        +31     
  Lines        461921     470345      +8424     
================================================
+ Hits         338180     347655      +9475     
+ Misses       102981     101257      -1724     
- Partials      20760      21433       +673

Flag	Coverage Δ
integration	`46.0066% <0.0000%> (?)`
unit	`72.4096% <100.0000%> (+0.0646%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.6910% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`46.0089% <ø> (+0.0150%)`	⬆️

Rustin170506

Thanks!

Rustin170506 · 2024-12-10T03:24:54Z

pkg/statistics/handle/storage/gc.go

 	rows, _, err := util.ExecRows(sctx, "select is_index, hist_id from mysql.stats_histograms where table_id = %?", physicalID)
 	if err != nil {
 		return errors.Trace(err)
 	}
 	// The table has already been deleted in stats and acknowledged to all tidb,
 	// we can safely remove the meta info now.
-	if len(rows) == 0 {
+	if len(rows) == 0 && !ok {


I guess this is never gonna be true.

Rustin170506

Thanks!

Some nits.

/hold

pkg/statistics/handle/storage/gc.go

winoros · 2024-12-11T11:07:37Z

/unhold

ti-chi-bot · 2024-12-11T11:43:13Z

@winoros: You cannot manually add or delete the reviewing state labels, only I and the tursted members have permission to do so.

In response to removing label named needs-1-more-lgtm.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

winoros · 2024-12-11T11:43:25Z

/merge

ti-chi-bot · 2024-12-11T11:43:28Z

@winoros: We have migrated to builtin LGTM and approve plugins for reviewing.

👉 Please use /approve when you want approve this pull request.

The changes announcement: LGTM plugin changes

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2024-12-11T11:46:22Z

[LGTM Timeline notifier]

Timeline:

2024-12-10 02:18:18.736568677 +0000 UTC m=+318488.825371220: ☑️ agreed by hawkingrei.
2024-12-10 03:25:19.431683264 +0000 UTC m=+322509.520485806: ✖️🔁 reset by Rustin170506.
2024-12-11 10:16:56.497600094 +0000 UTC m=+433606.586402631: ☑️ agreed by Rustin170506.
2024-12-11 11:46:21.686258117 +0000 UTC m=+438971.775060659: ☑️ agreed by hawkingrei.

winoros · 2024-12-11T13:09:28Z

/retest

ti-chi-bot · 2024-12-11T13:09:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AilinKid, hawkingrei, qw4990, Rustin170506

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:

~~pkg/statistics/OWNERS~~ [AilinKid,Rustin170506,hawkingrei,qw4990]

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

hawkingrei · 2024-12-12T05:33:00Z

/cherrypick release-7.5

ti-chi-bot · 2024-12-12T05:33:43Z

@hawkingrei: new pull request created to branch release-7.5: #58195.

In response to this:

/cherrypick release-7.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2025-01-08T08:12:53Z

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

winoros added 2 commits December 10, 2024 01:03

statistics: GCStats should not wrongly remove record of an existing t…

896765b

…able

add test

6629aae

ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/planner SIG: Planner labels Dec 9, 2024

winoros changed the title ~~statistics: GCStats should not wrongly remove record of an existing t…~~ statistics: GCStats should not wrongly remove record of an existing table Dec 9, 2024

hawkingrei approved these changes Dec 10, 2024

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Dec 10, 2024

Rustin170506 requested changes Dec 10, 2024

View reviewed changes

ti-chi-bot bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Dec 10, 2024

winoros added 4 commits December 10, 2024 21:44

fix the logic

317f6d7

Merge remote-tracking branch 'origin/master' into fix-gc-stats

0cabf42

update bazel_prepare

05d03a8

fix typo

bb841ee

Rustin170506 approved these changes Dec 11, 2024

View reviewed changes

pkg/statistics/handle/storage/gc.go Outdated Show resolved Hide resolved

pkg/statistics/handle/storage/gc.go Outdated Show resolved Hide resolved

ti-chi-bot bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Dec 11, 2024

address comments

4136245

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 11, 2024

winoros removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Dec 11, 2024

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Dec 11, 2024

hawkingrei approved these changes Dec 11, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Dec 11, 2024

AilinKid approved these changes Dec 11, 2024

View reviewed changes

qw4990 approved these changes Dec 11, 2024

View reviewed changes

ti-chi-bot bot merged commit d5b6b69 into pingcap:master Dec 11, 2024
24 checks passed

winoros deleted the fix-gc-stats branch December 11, 2024 13:45

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Dec 12, 2024

This is an automated cherry-pick of pingcap#58108

12954bd

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Dec 12, 2024

statistics: GCStats should not wrongly remove record of an existing table (#58108) #58195

Open

13 tasks

hawkingrei pushed a commit to ti-chi-bot/tidb that referenced this pull request Dec 12, 2024

This is an automated cherry-pick of pingcap#58108

002200f

Signed-off-by: ti-chi-bot <[email protected]>

lilinghai added the needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. label Jan 8, 2025

ti-chi-bot mentioned this pull request Jan 8, 2025

statistics: GCStats should not wrongly remove record of an existing table (#58108) #58796

Open

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

statistics: GCStats should not wrongly remove record of an existing table #58108

statistics: GCStats should not wrongly remove record of an existing table #58108

winoros commented Dec 9, 2024 •

edited

Loading

codecov bot commented Dec 9, 2024 •

edited

Loading

Rustin170506 left a comment

Rustin170506 Dec 10, 2024

Rustin170506 left a comment

winoros commented Dec 11, 2024

ti-chi-bot bot commented Dec 11, 2024

winoros commented Dec 11, 2024

ti-chi-bot bot commented Dec 11, 2024

ti-chi-bot bot commented Dec 11, 2024

winoros commented Dec 11, 2024

ti-chi-bot bot commented Dec 11, 2024

hawkingrei commented Dec 12, 2024

ti-chi-bot commented Dec 12, 2024

ti-chi-bot commented Jan 8, 2025

statistics: GCStats should not wrongly remove record of an existing table #58108

statistics: GCStats should not wrongly remove record of an existing table #58108

Conversation

winoros commented Dec 9, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

codecov bot commented Dec 9, 2024 • edited Loading

Codecov Report

Rustin170506 left a comment

Choose a reason for hiding this comment

Rustin170506 Dec 10, 2024

Choose a reason for hiding this comment

Rustin170506 left a comment

Choose a reason for hiding this comment

winoros commented Dec 11, 2024

ti-chi-bot bot commented Dec 11, 2024

winoros commented Dec 11, 2024

ti-chi-bot bot commented Dec 11, 2024

ti-chi-bot bot commented Dec 11, 2024

[LGTM Timeline notifier]

winoros commented Dec 11, 2024

ti-chi-bot bot commented Dec 11, 2024

hawkingrei commented Dec 12, 2024

ti-chi-bot commented Dec 12, 2024

ti-chi-bot commented Jan 8, 2025

winoros commented Dec 9, 2024 •

edited

Loading

codecov bot commented Dec 9, 2024 •

edited

Loading