*: keep in-memory resident for model.TableInfo with special attributes #53301

tiancaiamao · 2024-05-15T13:41:43Z

What problem does this PR solve?

Issue Number: ref #50959

Problem Summary:

SchemaTables() is a big problem for infoschema v2. It's slow, it cost a lot CPU resource, it make tidb memory thrash ...

What changed and how does it work?

I observe the pattern of call SchemaTables() API in our current codebase look-alike.
For ttl, tiflash and so on, they always iterate the SchemaTables and filter out some attributes.

	for _, dbName := range is.AllSchemaNames() {
		for _, tblInfo := range is.SchemaTableInfos(dbName) {
			if tblInfo.State != model.StatePublic || tblInfo.TTLInfo == nil {
				continue
			}
			...
		}
	}

So, what if we keep all the model.TableInfo with special attributes in-memory?
This could be a temporary solution, dirty and quick but easy to implement and verify.

In this commit, I add a new ListTablesWithSpecialAttribute() api to replace SchemaTableInfos.
All the data are in memory, so it is as fast as or even faster than infoschema v1.

Check List

Tests

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

The same data, memory usage after vs before:

We can see some jitters even with this change, it is caused by the statistics calling in this loop:

tidb/pkg/statistics/handle/bootstrap.go

Lines 88 to 109 in b9b330a

    
           sql := "select HIGH_PRIORITY version, table_id, modify_count, count from mysql.stats_meta" 
        
           rc, err := util.Exec(h.initStatsCtx, sql) 
        
           if err != nil { 
        
           	return nil, errors.Trace(err) 
        
           } 
        
           defer terror.Call(rc.Close) 
        
           tables, err := cache.NewStatsCacheImpl(h) 
        
           if err != nil { 
        
           	return nil, err 
        
           } 
        
           req := rc.NewChunk(nil) 
        
           iter := chunk.NewIterator4Chunk(req) 
        
           for { 
        
           	err := rc.Next(ctx, req) 
        
           	if err != nil { 
        
           		return nil, errors.Trace(err) 
        
           	} 
        
           	if req.NumRows() == 0 { 
        
           		break 
        
           	} 
        
           	h.initStatsMeta4Chunk(is, tables, iter) 
        
           }

And this commit only handle the memory in static state, when there are operations like show xxx or INFORMATION_SCHEMA.TABLES visiting, there may still be SchemaTables() calling cause memory thrash. I'll handle them later.

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.

None

tiprow · 2024-05-15T13:41:59Z

Hi @tiancaiamao. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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 kubernetes-sigs/prow repository.

codecov · 2024-05-15T14:10:36Z

Codecov Report

Attention: Patch coverage is 98.27586% with 2 lines in your changes missing coverage. Please review.

Project coverage is 55.2061%. Comparing base (c9c865c) to head (a54ac8c).
Report is 38 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #53301         +/-   ##
=================================================
- Coverage   72.4988%   55.2061%   -17.2928%     
=================================================
  Files          1506       1645        +139     
  Lines        430745     634236     +203491     
=================================================
+ Hits         312285     350137      +37852     
- Misses        99074     260616     +161542     
- Partials      19386      23483       +4097

Flag	Coverage Δ
integration	`35.8884% <69.0909%> (?)`
unit	`71.4182% <99.0909%> (-0.0677%)`	⬇️

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

Components	Coverage Δ
dumpling	`52.9656% <ø> (-1.0302%)`	⬇️
parser	`∅ <ø> (∅)`
br	`36.1727% <ø> (-5.6121%)`	⬇️

hawkingrei · 2024-05-15T14:48:41Z

/ok-to-test

D3Hunter · 2024-05-29T08:39:00Z

So, what if we keep all the model.TableInfo with special attributes in-memory?

can we only store those special attributes, not the whole table-info, as we have discussed in the coference?

tiancaiamao · 2024-05-29T09:03:36Z

So, what if we keep all the model.TableInfo with special attributes in-memory?

can we only store those special attributes, not the whole table-info, as we have discussed in the coference?

The callee still need the table info. For example, you can check here

tidb/pkg/ddl/ddl_tiflash_api.go

Lines 274 to 294 in 3ad726d

    
           func LoadTiFlashReplicaInfo(tblInfo *model.TableInfo, tableList *[]TiFlashReplicaStatus) { 
        
           	if tblInfo.TiFlashReplica == nil { 
        
           		// reject tables that has no tiflash replica such like `INFORMATION_SCHEMA` 
        
           		return 
        
           	} 
        
           	if pi := tblInfo.GetPartitionInfo(); pi != nil { 
        
           		for _, p := range pi.Definitions { 
        
           			logutil.DDLLogger().Debug(fmt.Sprintf("Table %v has partition %v\n", tblInfo.ID, p.ID)) 
        
           			*tableList = append(*tableList, TiFlashReplicaStatus{p.ID, 
        
           				tblInfo.TiFlashReplica.Count, tblInfo.TiFlashReplica.LocationLabels, tblInfo.TiFlashReplica.IsPartitionAvailable(p.ID), tblInfo.TiFlashReplica.Available, false, true}) 
        
           		} 
        
           		// partitions that in adding mid-state 
        
           		for _, p := range pi.AddingDefinitions { 
        
           			logutil.DDLLogger().Debug(fmt.Sprintf("Table %v has partition adding %v\n", tblInfo.ID, p.ID)) 
        
           			*tableList = append(*tableList, TiFlashReplicaStatus{p.ID, tblInfo.TiFlashReplica.Count, tblInfo.TiFlashReplica.LocationLabels, tblInfo.TiFlashReplica.IsPartitionAvailable(p.ID), tblInfo.TiFlashReplica.Available, true, true}) 
        
           		} 
        
           	} else { 
        
           		logutil.DDLLogger().Debug(fmt.Sprintf("Table %v has no partition\n", tblInfo.ID)) 
        
           		*tableList = append(*tableList, TiFlashReplicaStatus{tblInfo.ID, tblInfo.TiFlashReplica.Count, tblInfo.TiFlashReplica.LocationLabels, tblInfo.TiFlashReplica.Available, tblInfo.TiFlashReplica.Available, false, false}) 
        
           	} 
        
           }

Or do you mean just the table ID rather than *model.TableInfo? the pointer size itself is just a machine word.

@ywqzzy

wjhuang2016 · 2024-05-29T09:58:36Z

pkg/infoschema/infoschema.go

+	TableInfo []*model.TableInfo
+}
+
+func (is *infoSchema) ListTablesWithSpecialAttribute(filter specialAttributeFilter) <-chan tableInfoResult {


Do we need to add ctx?

Not now. If we need a ctx, then is.SchemaTableInfos etc all need it, and a lot other codes.

wjhuang2016 · 2024-05-29T10:07:58Z

So, what if we keep all the model.TableInfo with special attributes in-memory?

can we only store those special attributes, not the whole table-info, as we have discussed in the coference?

The callee still need the table info. For example, you can check here

tidb/pkg/ddl/ddl_tiflash_api.go

Lines 274 to 294 in 3ad726d

func LoadTiFlashReplicaInfo(tblInfo *model.TableInfo, tableList *[]TiFlashReplicaStatus) {

if tblInfo.TiFlashReplica == nil {

// reject tables that has no tiflash replica such like `INFORMATION_SCHEMA`

return

}

if pi := tblInfo.GetPartitionInfo(); pi != nil {

for _, p := range pi.Definitions {

logutil.DDLLogger().Debug(fmt.Sprintf("Table %v has partition %v\n", tblInfo.ID, p.ID))

*tableList = append(*tableList, TiFlashReplicaStatus{p.ID,

tblInfo.TiFlashReplica.Count, tblInfo.TiFlashReplica.LocationLabels, tblInfo.TiFlashReplica.IsPartitionAvailable(p.ID), tblInfo.TiFlashReplica.Available, false, true})

}

// partitions that in adding mid-state

for _, p := range pi.AddingDefinitions {

logutil.DDLLogger().Debug(fmt.Sprintf("Table %v has partition adding %v\n", tblInfo.ID, p.ID))

*tableList = append(*tableList, TiFlashReplicaStatus{p.ID, tblInfo.TiFlashReplica.Count, tblInfo.TiFlashReplica.LocationLabels, tblInfo.TiFlashReplica.IsPartitionAvailable(p.ID), tblInfo.TiFlashReplica.Available, true, true})

}

} else {

logutil.DDLLogger().Debug(fmt.Sprintf("Table %v has no partition\n", tblInfo.ID))

*tableList = append(*tableList, TiFlashReplicaStatus{tblInfo.ID, tblInfo.TiFlashReplica.Count, tblInfo.TiFlashReplica.LocationLabels, tblInfo.TiFlashReplica.Available, tblInfo.TiFlashReplica.Available, false, false})

}

}

Or do you mean just the table ID rather than *model.TableInfo? the pointer size itself is just a machine word.

@ywqzzy

Store these infos may consume too much memory.

tiancaiamao · 2024-05-30T11:26:13Z

So, what if we keep all the model.TableInfo with special attributes in-memory?

can we only store those special attributes, not the whole table-info, as we have discussed in the coference?

The callee still need the table info. For example, you can check here

tidb/pkg/ddl/ddl_tiflash_api.go

Lines 274 to 294 in 3ad726d

func LoadTiFlashReplicaInfo(tblInfo *model.TableInfo, tableList *[]TiFlashReplicaStatus) {

if tblInfo.TiFlashReplica == nil {

// reject tables that has no tiflash replica such like `INFORMATION_SCHEMA`

return

}

if pi := tblInfo.GetPartitionInfo(); pi != nil {

for _, p := range pi.Definitions {

logutil.DDLLogger().Debug(fmt.Sprintf("Table %v has partition %v\n", tblInfo.ID, p.ID))

*tableList = append(*tableList, TiFlashReplicaStatus{p.ID,

tblInfo.TiFlashReplica.Count, tblInfo.TiFlashReplica.LocationLabels, tblInfo.TiFlashReplica.IsPartitionAvailable(p.ID), tblInfo.TiFlashReplica.Available, false, true})

}

// partitions that in adding mid-state

for _, p := range pi.AddingDefinitions {

logutil.DDLLogger().Debug(fmt.Sprintf("Table %v has partition adding %v\n", tblInfo.ID, p.ID))

*tableList = append(*tableList, TiFlashReplicaStatus{p.ID, tblInfo.TiFlashReplica.Count, tblInfo.TiFlashReplica.LocationLabels, tblInfo.TiFlashReplica.IsPartitionAvailable(p.ID), tblInfo.TiFlashReplica.Available, true, true})

}

} else {

logutil.DDLLogger().Debug(fmt.Sprintf("Table %v has no partition\n", tblInfo.ID))

*tableList = append(*tableList, TiFlashReplicaStatus{tblInfo.ID, tblInfo.TiFlashReplica.Count, tblInfo.TiFlashReplica.LocationLabels, tblInfo.TiFlashReplica.Available, tblInfo.TiFlashReplica.Available, false, false})

}

}

Or do you mean just the table ID rather than *model.TableInfo? the pointer size itself is just a machine word.
@ywqzzy

Store these infos may consume too much memory.

If we don't store them, the we go for the Data Dictionary solution. @wjhuang2016

wjhuang2016 · 2024-05-30T12:59:29Z

So, what if we keep all the model.TableInfo with special attributes in-memory?

can we only store those special attributes, not the whole table-info, as we have discussed in the coference?

The callee still need the table info. For example, you can check here

tidb/pkg/ddl/ddl_tiflash_api.go

Lines 274 to 294 in 3ad726d

func LoadTiFlashReplicaInfo(tblInfo *model.TableInfo, tableList *[]TiFlashReplicaStatus) {

if tblInfo.TiFlashReplica == nil {

// reject tables that has no tiflash replica such like `INFORMATION_SCHEMA`

return

}

if pi := tblInfo.GetPartitionInfo(); pi != nil {

for _, p := range pi.Definitions {

logutil.DDLLogger().Debug(fmt.Sprintf("Table %v has partition %v\n", tblInfo.ID, p.ID))

*tableList = append(*tableList, TiFlashReplicaStatus{p.ID,

tblInfo.TiFlashReplica.Count, tblInfo.TiFlashReplica.LocationLabels, tblInfo.TiFlashReplica.IsPartitionAvailable(p.ID), tblInfo.TiFlashReplica.Available, false, true})

}

// partitions that in adding mid-state

for _, p := range pi.AddingDefinitions {

logutil.DDLLogger().Debug(fmt.Sprintf("Table %v has partition adding %v\n", tblInfo.ID, p.ID))

*tableList = append(*tableList, TiFlashReplicaStatus{p.ID, tblInfo.TiFlashReplica.Count, tblInfo.TiFlashReplica.LocationLabels, tblInfo.TiFlashReplica.IsPartitionAvailable(p.ID), tblInfo.TiFlashReplica.Available, true, true})

}

} else {

logutil.DDLLogger().Debug(fmt.Sprintf("Table %v has no partition\n", tblInfo.ID))

*tableList = append(*tableList, TiFlashReplicaStatus{tblInfo.ID, tblInfo.TiFlashReplica.Count, tblInfo.TiFlashReplica.LocationLabels, tblInfo.TiFlashReplica.Available, tblInfo.TiFlashReplica.Available, false, false})

}

}

Or do you mean just the table ID rather than *model.TableInfo? the pointer size itself is just a machine word.
@ywqzzy

Store these infos may consume too much memory.

If we don't store them, the we go for the Data Dictionary solution. @wjhuang2016

Why do you think it's needed to store them? Performance or Code complexity?
I think it's better not to mix all special attributes together, if so you need to filter them again during retrieval.
Besides, is it necessary to use schemaVersion and tomb if the newest schema is always used?
Another consideration is that we won't do "Full load" during bootstrap later, then only the needed table infos will be unmarshalled and loaded into memory.

pkg/infoschema/infoschema.go

pkg/infoschema/infoschema_v2.go

D3Hunter · 2024-05-31T10:42:53Z

pkg/infoschema/infoschema.go

+	TableInfo []*model.TableInfo
+}
+
+func (is *infoSchema) ListTablesWithSpecialAttribute(filter specialAttributeFilter) <-chan tableInfoResult {


Suggested change

func (is *infoSchema) ListTablesWithSpecialAttribute(filter specialAttributeFilter) <-chan tableInfoResult {

func (is *infoSchema) ListTablesWithSpecialAttribute(dbName string, filter specialAttributeFilter) <-chan tableInfoResult {

can we change it to get all tables with matched attributes for a db? so we don't to have a routine to do it. it's more complex and error-prone now

This makes the streaming API possible. For example, if we don't store the special attribute and need to fetch data from tikv, then return all data []XXX may use much more memory then this API setting. @D3Hunter

ti-chi-bot · 2024-06-04T04:07:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: D3Hunter, wjhuang2016

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~~ [D3Hunter,wjhuang2016]
~~pkg/ddl/OWNERS~~ [wjhuang2016]
~~pkg/infoschema/OWNERS~~ [D3Hunter,wjhuang2016]

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 · 2024-06-04T04:07:13Z

[LGTM Timeline notifier]

Timeline:

2024-05-31 09:12:02.164889317 +0000 UTC m=+3026875.922024888: ☑️ agreed by wjhuang2016.
2024-06-04 04:07:12.690722359 +0000 UTC m=+3354186.447857933: ☑️ agreed by D3Hunter.

D3Hunter · 2024-06-04T04:07:56Z

/hold

please fix the callsite of the ListTablesWithSpecialAttribute, you can unhold after change

tiancaiamao · 2024-06-04T05:05:13Z

/unhold

tiancaiamao added 6 commits May 10, 2024 16:07

save the current work

81a036a

leak of DBInfo.Tables

0661e1f

list with filter api

605e3e4

Merge branch 'master' into tune

4a0d074

cleanup

8775b94

add test

b6ae044

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 15, 2024

tiancaiamao requested review from ywqzzy, wjhuang2016 and GMHDBJD May 15, 2024 13:45

make lint happy

b52d657

fix test

9439c37

ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label May 15, 2024

tiancaiamao added 3 commits May 16, 2024 09:15

make test stable

a0cbc71

Merge branch 'master' into tune

0ef9193

fix ci

9e9b725

tiancaiamao mentioned this pull request May 21, 2024

domain,infoschema: put the @@tidb_schema_cache_size variable into effect #53424

Merged

13 tasks

Merge branch 'master' into tune

3ad726d

make fmt

2ad81ec

wjhuang2016 reviewed May 29, 2024

View reviewed changes

wjhuang2016 approved these changes May 31, 2024

View reviewed changes

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

D3Hunter reviewed May 31, 2024

View reviewed changes

address comment

fef226d

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

tiancaiamao added 3 commits June 4, 2024 05:06

fix build

76ad33e

make fmt

240eb88

address comment

a54ac8c

D3Hunter approved these changes Jun 4, 2024

View reviewed changes

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

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

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 Jun 4, 2024

ti-chi-bot bot merged commit 625516b into pingcap:master Jun 4, 2024
23 checks passed

tiancaiamao deleted the tune branch June 4, 2024 08:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: keep in-memory resident for model.TableInfo with special attributes #53301

*: keep in-memory resident for model.TableInfo with special attributes #53301

tiancaiamao commented May 15, 2024 •

edited

Loading

tiprow bot commented May 15, 2024

codecov bot commented May 15, 2024 •

edited

Loading

hawkingrei commented May 15, 2024

D3Hunter commented May 29, 2024

tiancaiamao commented May 29, 2024

wjhuang2016 May 29, 2024

tiancaiamao May 30, 2024

wjhuang2016 commented May 29, 2024

tiancaiamao commented May 30, 2024

wjhuang2016 commented May 30, 2024

D3Hunter May 31, 2024 •

edited

Loading

tiancaiamao Jun 3, 2024

ti-chi-bot bot commented Jun 4, 2024

ti-chi-bot bot commented Jun 4, 2024

D3Hunter commented Jun 4, 2024 •

edited

Loading

tiancaiamao commented Jun 4, 2024

	sql := "select HIGH_PRIORITY version, table_id, modify_count, count from mysql.stats_meta"
	rc, err := util.Exec(h.initStatsCtx, sql)
	if err != nil {
	return nil, errors.Trace(err)
	}
	defer terror.Call(rc.Close)
	tables, err := cache.NewStatsCacheImpl(h)
	if err != nil {
	return nil, err
	}
	req := rc.NewChunk(nil)
	iter := chunk.NewIterator4Chunk(req)
	for {
	err := rc.Next(ctx, req)
	if err != nil {
	return nil, errors.Trace(err)
	}
	if req.NumRows() == 0 {
	break
	}
	h.initStatsMeta4Chunk(is, tables, iter)
	}

	func (is *infoSchema) ListTablesWithSpecialAttribute(filter specialAttributeFilter) <-chan tableInfoResult {
	func (is *infoSchema) ListTablesWithSpecialAttribute(dbName string, filter specialAttributeFilter) <-chan tableInfoResult {

*: keep in-memory resident for model.TableInfo with special attributes #53301

*: keep in-memory resident for model.TableInfo with special attributes #53301

Conversation

tiancaiamao commented May 15, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented May 15, 2024

codecov bot commented May 15, 2024 • edited Loading

Codecov Report

hawkingrei commented May 15, 2024

D3Hunter commented May 29, 2024

tiancaiamao commented May 29, 2024

wjhuang2016 May 29, 2024

Choose a reason for hiding this comment

tiancaiamao May 30, 2024

Choose a reason for hiding this comment

wjhuang2016 commented May 29, 2024

tiancaiamao commented May 30, 2024

wjhuang2016 commented May 30, 2024

D3Hunter May 31, 2024 • edited Loading

Choose a reason for hiding this comment

tiancaiamao Jun 3, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Jun 4, 2024

ti-chi-bot bot commented Jun 4, 2024

[LGTM Timeline notifier]

D3Hunter commented Jun 4, 2024 • edited Loading

tiancaiamao commented Jun 4, 2024

tiancaiamao commented May 15, 2024 •

edited

Loading

codecov bot commented May 15, 2024 •

edited

Loading

D3Hunter May 31, 2024 •

edited

Loading

D3Hunter commented Jun 4, 2024 •

edited

Loading