*: activate txn for query on infoschema tables by tiancaiamao · Pull Request #57937 · pingcap/tidb

tiancaiamao · 2024-12-03T08:52:11Z

What problem does this PR solve?

Issue Number: close #57874

Problem Summary:

The GC should be blocked by query.

What changed and how does it work?

There are two places that the transaction should blcok GC:

The query "SELECT COUNT(1) as c FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_TYPE='SEQUENCE'" itself
The internal transaction meta.ListTables invoked by SchemaTableInfos() API

This commit just fix the first one. It should be enough to fix issue 57874.
The root cause is that whe we query on infoschema tables, the transaction is not activated.
So the session process info would not be collected to block GC.

The second issue is caused by the fact that the meta API call does not block GC.
I don't take investigation on it further more, meta API is limited used.

Check List

Tests

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

Modify the code and build tidb, check no more "GC life time is shorter than transaction duration" error.

diff --git a/pkg/planner/core/memtable_infoschema_extractor.go b/pkg/planner/core/memtable_infoschema_extractor.go
index c527dde968..34cabb3677 100644
--- a/pkg/planner/core/memtable_infoschema_extractor.go
+++ b/pkg/planner/core/memtable_infoschema_extractor.go
@@ -15,6 +15,7 @@
 package core
 
 import (
+       "time"
        "bytes"
        "context"
        "fmt"
@@ -184,6 +185,7 @@ func (e *InfoSchemaBaseExtractor) ListSchemasAndTables(
                return findTableAndSchemaByName(ctx, is, schemas, tableNames)
        }
        schemas := e.ListSchemas(is)
+       time.Sleep(12 * time.Minute)
        return listTablesForEachSchema(ctx, e, is, schemas)
 }

mysql> set @@tidb_mem_quota_query = 12*1024*1024*1024;
Query OK, 0 rows affected (0.00 sec)

mysql> select count(tidb_table_id) from information_schema.tables where table_name != 'test';
+----------------------+
| count(tidb_table_id) |
+----------------------+
|              1000818 |
+----------------------+
1 row in set (12 min 28.84 sec)

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-12-03T08:52:27Z

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.

Details

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.

pkg/executor/memtable_reader.go

Co-authored-by: lance6716 <lance6716@gmail.com>

tiancaiamao · 2024-12-03T09:14:09Z

/hold

tiancaiamao · 2024-12-03T09:42:33Z

The second case is still risky after this fix.

If we take a latest infoschema, and use it, continue for 10min (or maybe less)
The GC error may still happen because the internal txn does not block GC.

tiancaiamao · 2024-12-03T09:43:57Z

/unhold

codecov · 2024-12-03T12:38:56Z

Codecov Report

Attention: Patch coverage is 75.00000% with 8 lines in your changes missing coverage. Please review.

Project coverage is 73.6233%. Comparing base (5598c11) to head (6e4ab15).
Report is 6 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #57937        +/-   ##
================================================
+ Coverage   73.1836%   73.6233%   +0.4396%     
================================================
  Files          1671       1671                
  Lines        460712     461624       +912     
================================================
+ Hits         337166     339863      +2697     
+ Misses       102825     101035      -1790     
- Partials      20721      20726         +5

Flag	Coverage Δ
integration	`43.2697% <75.0000%> (?)`
unit	`72.3051% <75.0000%> (-0.0172%)`	⬇️

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

Components	Coverage Δ
dumpling	`52.6910% <ø> (-0.0764%)`	⬇️
parser	`∅ <ø> (∅)`
br	`46.0134% <ø> (-0.0121%)`	⬇️

lance6716

Can you find the place that min TS of active txn ? We can write a testkit test to check the minTS really takes effect with this PR

pkg/executor/memtable_reader.go

ti-chi-bot · 2024-12-03T14:21:43Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [lance6716,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-12-03T14:21:46Z

[LGTM Timeline notifier]

Timeline:

2024-12-03 08:53:28.343175453 +0000 UTC m=+1144995.962829968: ☑️ agreed by lance6716.
2024-12-03 14:21:45.598312086 +0000 UTC m=+1164693.217966596: ☑️ agreed by wjhuang2016.

wjhuang2016 · 2024-12-03T14:22:54Z

/hold wait for adding comment.

tiancaiamao · 2024-12-03T15:07:34Z

Can you find the place that min TS of active txn ? We can write a testkit test to check the minTS really takes effect with this PR

It's here @lance6716

tidb/pkg/domain/infosync/info.go

Lines 760 to 765 in d021648

    
           func (is *InfoSyncer) ReportMinStartTS(store kv.Storage) { 
        
           	sm := is.GetSessionManager() 
        
           	if sm == nil { 
        
           		return 
        
           	} 
        
           	pl := sm.ShowProcessList()

The start ts is collected by geting ProcessInfo.
Actually, I tired to add some tesr to cover that executing "select * from information_schema.xxx", the start ts of session process info is non-zero.
But I find the failpoint is ugly and the cover check looks like "prove my mother is my mother", so I give up.

func TestInfoschemaTableActivateTxn(t *testing.T) {
	store := testkit.CreateMockStore(t)
	tk := testkit.NewTestKit(t, store)

	// This test checks that query on infoschema tables should activate txn.
	testfailpoint.EnableCall(t, "github.com/pingcap/tidb/pkg/executor/inspectSession", func(sctx sessionctx.Context) {
		pi := sctx.ShowProcess()
		require.True(t, pi.CurTxnStartTS > 0)
		fmt.Println("============================================================")
	})


	for _, sql := range []string{
		"select count(tidb_table_id) > 0 from information_schema.tables where table_schema != 'test'",
		"select count(*) from information_schema.sequences",
	}{
		tk.MustQuery(sql)
	}

	require.True(t, called)
}

(If e.Ctx().Txn(true) is called, start ts is non-zero, otherwise it' zero)

tiancaiamao · 2024-12-03T16:57:03Z

/unlock

tiancaiamao · 2024-12-03T16:57:29Z

/unhold

ti-chi-bot · 2024-12-03T17:02:10Z

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

tiancaiamao · 2024-12-03T17:13:02Z

Create an issue #57952 to trace the second problem.
That one is not blocking, I list as en enhancement, as most of the cases should have been avoided by this commit.

close #57874

* skip TestIndexJoin31494 * fix * global sort: add boundaries to split keys when generating plan (pingcap#58323) (pingcap#58356) * statistics: get right max table id when to init stats (pingcap#58280) (pingcap#58298) * executor: Fix the parse problematic slow log panic issue due to empty … * statstics: trigger evict by the timer (pingcap#58027) (pingcap#58268) * br: make table existence check unified on different br client (pingcap#58211) (pingcap#58262) * log backup: use global checkpoint ts as source of truth (pingcap#58135) (pingcap#58265) * executor: skip execution when build query for VIEW in I_S (pingcap#58203) (pingcap#58236) * statistics: copy stats when to update it for avoiding data race (pingcap#5810… * domain,infoschema: make infoschema activity block GC safepoint advanci… * planner: handle panic when loading bindings at startup (pingcap#58017) (pingcap#58035) * statistics: right deal with error for reading stats from storage (pingcap#58… * statistics: lite init used wrong value to build table stats ver (pingcap#5802… * lightning, ddl: set TS to engineMeta after ResetEngineSkipAllocTS (pingcap#5… * *: avoid unlock of unlocked mutex panic on TableDeltaMap (pingcap#57799) (pingcap#57997) * ddl: handle context done after sending DDL jobs (pingcap#57945) (pingcap#57989) * *: activate txn for query on infoschema tables (pingcap#57937) (pingcap#57951) * lightning: add PK to internal tables (pingcap#57480) (pingcap#57932) * statistics: correct behavior of non-lite InitStats and stats sync load… * statistics: avoid stats meta full load after table analysis (pingcap#57756) (pingcap#57911) * dumpling: use I_S to get table list for TiDB and add database to WHERE… * br: fix insert gc failed due to slow schema reload (pingcap#57742) (pingcap#57907) * statistics: do not record historical stats meta if the table is locked… * metrics: remove the filled colors (pingcap#57838) (pingcap#57866) * planner: use TableInfo.DBID to locate schema (pingcap#57785) (pingcap#57870) * *: support cancel query like 'select * from information_schema.tables'… * session: make `TxnInfo()` return even if process info is empty (pingcap#57044) (pingcap#57161) * ddl: Fixed partitioning a non-partitioned table with placement rules (… * *: Reorg partition fix delete ranges and handling non-clustered tables… * executor: fix query infoschema.tables table_schema/table_name with fil… * ddl: check context done in isReorgRunnable function (pingcap#57813) (pingcap#57820) * ddl: fix ExistsTableRow and add tests for skip reorg checks (pingcap#57778) (pingcap#57801) * *: Fix for TRUNCATE PARTITION and Global Index (pingcap#57724) * br: prompt k8s.io/api version (pingcap#57791) (pingcap#57802) * statistics: fix some problem related to stats async load (pingcap#57723) (pingcap#57775) * expression: fix wrong calculation order of `radians` (pingcap#57672) (pingcap#57688) * statistics: rightly deal with timout when to send sync load (pingcap#57712) (pingcap#57751) * ddl: `tidb_scatter_region` variable supports setting value in both upp… * planner: fix that vector index output empty result when pk is non-int … * ddl: dynamically adjusting the max write speed of reorganization job (… * executor: fix hang in hash agg when exceeding memory limit leads to pa… * statistics: use infoschema api to get table info (pingcap#57574) (pingcap#57614) * planner: Use realtimeRowCount when all topN collected (pingcap#56848) (pingcap#57689) * statistics: handle deleted tables correctly in the PQ (pingcap#57649) (pingcap#57674) * backup: reset timeout on store level (pingcap#55526) (pingcap#57667) * planner/core: fix a wrong privilege check for CTE & UPDATE statement (…

*: activate txn for query on infoschema tables

ec3fff0

ti-chi-bot bot added do-not-merge/needs-triage-completed release-note-none Denotes a PR that doesn't merit a release note. needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 3, 2024

tiancaiamao requested review from lance6716 and wjhuang2016 December 3, 2024 08:53

lance6716 approved these changes Dec 3, 2024

View reviewed changes

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

tiancaiamao requested a review from MyonKeminta December 3, 2024 08:53

tiancaiamao mentioned this pull request Dec 3, 2024

"select * from information_schema.tidb_trx" error #57916

Closed

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label Dec 3, 2024

lance6716 reviewed Dec 3, 2024

View reviewed changes

pkg/executor/memtable_reader.go Outdated Show resolved Hide resolved

Update pkg/executor/memtable_reader.go

ecdf605

Co-authored-by: lance6716 <lance6716@gmail.com>

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 Dec 3, 2024

prefer the exact infoschema to the latest infoschema

8f30e74

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 3, 2024

tiancaiamao added 2 commits December 3, 2024 17:50

make lint happy

f5e85ef

fix CI

ceb89ce

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 3, 2024

tiancaiamao requested a review from lance6716 December 3, 2024 12:21

lance6716 reviewed Dec 3, 2024

View reviewed changes

pkg/executor/memtable_reader.go Outdated Show resolved Hide resolved

fix CI

469639c

wjhuang2016 approved these changes Dec 3, 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 3, 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 Dec 3, 2024

address comment

6e4ab15

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 3, 2024

ti-chi-bot bot merged commit 9812d85 into pingcap:master Dec 3, 2024

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

*: activate txn for query on infoschema tables (#57937) #57951

Merged

13 tasks

tiancaiamao deleted the issue57874 branch December 3, 2024 17:03

ti-chi-bot bot pushed a commit that referenced this pull request Dec 4, 2024

*: activate txn for query on infoschema tables (#57937) (#57951)

1df27e0

close #57874

owlsome2501 mentioned this pull request Dec 5, 2024

There is a 57.7% performance regression in corebanking after PR#57937 #58028

Open

wjhuang2016 mentioned this pull request Sep 23, 2025

dumpling does not export table data with --snapshot when table has been dropped #63600

Closed

Conversation

tiancaiamao commented Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

Uh oh!

tiprow bot commented Dec 3, 2024

Uh oh!

Uh oh!

tiancaiamao commented Dec 3, 2024

Uh oh!

tiancaiamao commented Dec 3, 2024

Uh oh!

tiancaiamao commented Dec 3, 2024

Uh oh!

codecov bot commented Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lance6716 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ti-chi-bot bot commented Dec 3, 2024

Uh oh!

ti-chi-bot bot commented Dec 3, 2024

[LGTM Timeline notifier]

Uh oh!

wjhuang2016 commented Dec 3, 2024

Uh oh!

tiancaiamao commented Dec 3, 2024

Uh oh!

tiancaiamao commented Dec 3, 2024

Uh oh!

tiancaiamao commented Dec 3, 2024

Uh oh!

ti-chi-bot commented Dec 3, 2024

Uh oh!

tiancaiamao commented Dec 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tiancaiamao commented Dec 3, 2024 •

edited

Loading

codecov bot commented Dec 3, 2024 •

edited

Loading