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

infoschema: Add ALL_SQL_DIGESTS field to the TIDB_TRX table #24863

Merged
merged 14 commits into from
May 27, 2021

Conversation

MyonKeminta
Copy link
Contributor

What problem does this PR solve?

Problem Summary:
The TIDB_TRX table is part of Lock View , introduced in #22908 . However one of the important fields ALL_SQLS is not finished in that PR. This PR adds this field, and also avoids ALL_SQLS, DIGEST, State being inconsistent due to multiple atomic loads.

What is changed and how it works?

What's Changed: Makes the TxnInfo struct a hybrid thing of mutable fields (updated with atomic operations) and immutable fields (updated by replacing the whole struct). When updating the SQL Digest, the whole struct will be replaced. However, the other fields that are not so important (len and size) is still loaded by multiple atomic operations. Then the AllSQLs information is added to the struct as an immutable field.

This PR also added more tests.

Related changes

Check List

Tests

  • Unit test

Side effects

  • Performance regression
    • Consumes more CPU
    • Consumes more MEM

Release note

  • No release note.

@MyonKeminta MyonKeminta added the sig/transaction SIG:Transaction label May 24, 2021
@ti-chi-bot ti-chi-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 24, 2021
@MyonKeminta
Copy link
Contributor Author

/run-check_dev_2

@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label May 24, 2021
session/session.go Outdated Show resolved Hide resolved
session/txninfo/txn_info.go Outdated Show resolved Hide resolved
session/txn.go Show resolved Hide resolved
@MyonKeminta MyonKeminta mentioned this pull request May 25, 2021
20 tasks
@MyonKeminta
Copy link
Contributor Author

MyonKeminta commented May 25, 2021

Is the bench command still available?
/bench

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2021
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2021
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
@MyonKeminta MyonKeminta requested a review from a team as a code owner May 26, 2021 08:37
@MyonKeminta MyonKeminta requested review from lzmhhh123 and removed request for a team May 26, 2021 08:37
@MyonKeminta MyonKeminta changed the title infoschema: Add ALL_SQLS field to the TIDB_TRX table infoschema: Add ALL_SQL_DIGESTS field to the TIDB_TRX table May 26, 2021
@github-actions github-actions bot added the sig/execution SIG execution label May 26, 2021
@MyonKeminta
Copy link
Contributor Author

/run-all-tests

session/txn.go Outdated
return
}

info := txn.txnInfo.GetSnapInfo()
Copy link
Contributor

@longfangsong longfangsong May 26, 2021

Choose a reason for hiding this comment

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

Though GetSnapInfo is thread safe, accessing txn.txnInfo is not, so here's a datarace with txn.txnInfo = info in storeTxnInfo.

Can we use atomicTxnInfo all the time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

onStmtStart is expected to run within the transaction's thread. The purpose I keeps the txnInfo field is to avoid atomic loading every time it's used, and I expect the txnInfo is only used within the transaction's thread and the atomicTxnInfo is only used for fetching information concurrently. Perhaps this is a bad idea. 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry. I just notice the race reported by CI. It's weird to me that onStmtStart and onStmtEnd is invoked concurrently...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test TestAmendForUniqueIndex in pessimistic_test.go is executing two queries concurrently in a single tk... @cfzjywxk Is this this kind of usage allowed..?

	go func() {
		var err error
		err = tk2.ExecToErr("begin pessimistic")
		if err != nil {
			errCh <- err
			return
		}
		err = tk2.ExecToErr("insert into t values(5, 5)")
		if err != nil {
			errCh <- err
			return
		}
		err = tk2.ExecToErr("delete from t where id = 5")
		if err != nil {
			errCh <- err
			return
		}
		// let commit in tk start.
		errCh <- err
		time.Sleep(time.Millisecond * 100)
		err = tk2.ExecToErr("commit")
		errCh <- err
	}()
	err = <-errCh
	c.Assert(err, Equals, nil)
	tk.MustExec("commit")
	tk2.MustExec("admin check table t")
	err = <-errCh
	c.Assert(err, Equals, nil)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's unexpected usage, need to fix.

Copy link
Contributor

@cfzjywxk cfzjywxk 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 added the status/LGT1 Indicates that a PR has LGTM 1. label May 27, 2021
Copy link
Contributor

@longfangsong longfangsong 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
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • cfzjywxk
  • longfangsong

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 27, 2021
@MyonKeminta
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 43cd7b4

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 27, 2021
@ti-chi-bot
Copy link
Member

@MyonKeminta: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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.

@MyonKeminta
Copy link
Contributor Author

/run-unit-test

@MyonKeminta
Copy link
Contributor Author

/merge

@MyonKeminta
Copy link
Contributor Author

What's wrong with TestMemoryUsageAlarmVariable

[2021-05-27T11:29:47.765Z] ----------------------------------------------------------------------
[2021-05-27T11:29:47.765Z] FAIL: session_test.go:3812: testSessionSuite2.TestMemoryUsageAlarmVariable
[2021-05-27T11:29:47.765Z] 
[2021-05-27T11:29:47.765Z] session_test.go:3820:
[2021-05-27T11:29:47.765Z]     tk.MustQuery("select @@session.tidb_memory_usage_alarm_ratio").Check(testkit.Rows("0.7"))
[2021-05-27T11:29:47.765Z] /home/jenkins/agent/workspace/tidb_ghpr_unit_test/go/src/github.com/pingcap/tidb/util/testkit/testkit.go:63:
[2021-05-27T11:29:47.765Z]     res.c.Assert(resBuff.String(), check.Equals, needBuff.String(), res.comment)
[2021-05-27T11:29:47.765Z] ... obtained string = "[0.8]\n"
[2021-05-27T11:29:47.765Z] ... expected string = "[0.7]\n"
[2021-05-27T11:29:47.766Z] ... sql:select @@session.tidb_memory_usage_alarm_ratio, args:[]

@MyonKeminta
Copy link
Contributor Author

/merge

@ti-chi-bot ti-chi-bot merged commit c3a27c9 into pingcap:master May 27, 2021
@MyonKeminta MyonKeminta deleted the m/tidb-trx-dev branch May 27, 2021 11:50
txn.initStmtBuf()
atomic.StoreUint64(&txn.EntriesCount, uint64(txn.Transaction.Len()))
atomic.StoreUint64(&txn.EntriesSize, uint64(txn.Transaction.Size()))
txn.recreateTxnInfo(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why atomic? when will the changeInvalidToValid() function been called concurrently? @MyonKeminta

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should not be called concurrently, but the content of the txnInfo may be retrieved by another thread outside the session.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution sig/sql-infra SIG: SQL Infra sig/transaction SIG:Transaction size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants