txn: support read consistency read with ts checking #32922

cfzjywxk · 2022-03-08T12:25:37Z

What problem does this PR solve?

Issue Number: close #33159

Problem Summary:
Try to support the read-consistency read with tso checking optimization.

What is changed and how it works?

A new execution path for in-transaction read-consistency read is added, in which the first time execution last valid ts will be used as the for_update_ts. If the read check has failed, the select statement will be retried only if no result chunk has been sent back to the mysql client yet , a new global ts will be fetched doing retry.
A new system variable tidb_rc_read_check_ts is added to control whether the new path will be used, the default value is false.

Check List

Tests

Unit test

Side effects

Documentation

Affects user behaviors
Contains variable changes

Release note

Support the read-consistency read with ts checking optimization.

ti-chi-bot · 2022-03-08T12:25:38Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

MyonKeminta
you06

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 submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

you06 · 2022-03-09T08:28:43Z

server/conn.go

@@ -1849,6 +1849,17 @@ func (cc *clientConn) handleQuery(ctx context.Context, sql string) (err error) {
 		}
 		retryable, err = cc.handleStmt(ctx, stmt, parserWarns, i == len(stmts)-1)
 		if err != nil {
+			if retryable && cc.ctx.GetSessionVars().IsRcCheckTsRetryable(err) {


If it's not retryable, will it raise an error to the client?

Yes, the write conflict error will be returned.

server/conn_stmt.go

you06 · 2022-03-09T08:49:28Z

distsql/request_builder.go

@@ -246,6 +246,8 @@ func (builder *RequestBuilder) SetFromSessionVars(sv *variable.SessionVars) *Req
 	}
 	if sv.StmtCtx.WeakConsistency {
 		builder.Request.IsolationLevel = kv.RC
+	} else if sv.StmtCtx.RCCheckTS {
+		builder.Request.IsolationLevel = kv.RCCheckTS


So weak consistency will override it, though both of them are barely used...

It's ok to override if the tikv-rc is enabled as no transaction sematics is kept.

executor/executor.go

sessionctx/variable/session.go

TonsnakeLin · 2022-03-09T11:55:04Z

executor/builder.go

+		if rcReadTS == 0 {
+			rcReadTS = b.ctx.GetSessionVars().TxnCtx.StartTS
+		}
+		return UpdateForUpdateTS(b.ctx, rcReadTS)


If rcReadTS != 0, do we need call UpdateForUpdateTS ? Can we optimize it to call UpdateForUpdateTS only once here ?

Only SELECT statement call function refreshForUpdateTSForRC?

It's expected to update the for_update_ts each time using the last valid read_ts. refreshForUpdateTSForRC will be used by write statements too in updateForUpdateTSIfNeeded.

TonsnakeLin · 2022-03-09T12:08:56Z

executor/executor.go

@@ -1881,6 +1881,12 @@ func ResetContextOfStmt(ctx sessionctx.Context, s ast.StmtNode) (err error) {
 			sc.NotFillCache = !opts.SQLCache
 		}
 		sc.WeakConsistency = isWeakConsistencyRead(ctx, stmt)
+		// Try to mark the `RCCheckTS` flag for the first time execution of in-transaction read requests
+		// using read-consistency isolation level.
+		if ctx.GetSessionVars().ConnectionID > 0 && ctx.GetSessionVars().RcReadCheckTS &&


Do we need add a transaction !AutoCommitTxn() && SelectCmd here?

Select is already guaranteed in the above case condition. !AutoCommitTxn() is needed I'll add it.

TonsnakeLin · 2022-03-09T12:16:50Z

executor/point_get.go

@@ -156,6 +156,9 @@ func (e *PointGetExecutor) Open(context.Context) error {
 	} else {
 		e.snapshot = e.ctx.GetSnapshotWithTS(snapshotTS)
 	}
+	if e.ctx.GetSessionVars().StmtCtx.RCCheckTS {


Why only PointGet and BatachPointGet need to process for RCCheckTS specially ?

These cop executor builder will set the isolation flag but these two executors use txn snapshot directly.

server/conn.go

TonsnakeLin · 2022-03-09T13:28:21Z

sessionctx/variable/session.go

+		return false
+	}
+	// The `RCCheckTS` flag of `stmtCtx` is reset d
+	return s.RcReadCheckTS && s.StmtCtx.RCCheckTS && errors.ErrorEqual(err, kv.ErrWriteConflict)


We reuse the error code kv.ErrWriteConflict, does it affect the write sqls behavior when write-conflict occur really.

This error is newly introduced to the SELECT statements I think it'll not affect the original write statment path.

May WriteConflict error be met during pessimistic lock phase when executing a DML statement, and then checked by this function?

I've added another condition judgement checking if it's read only statement.

TonsnakeLin · 2022-03-09T13:36:12Z

store/copr/coprocessor.go

@@ -977,6 +977,10 @@ func (worker *copIteratorWorker) handleCopResponse(bo *Backoffer, rpcCtx *tikv.R
 			zap.Uint64("regionID", task.region.GetID()),
 			zap.String("storeAddr", task.storeAddr),
 			zap.Error(err))
+		if strings.Contains(err.Error(), "write conflict") {


Do we need add a limitation that is a "Rc Check Ts read" ? I mean whether some other scenes make a "write conflict" here.

In the read path only the added path may return other error contains "write conflit", it's a bit difficult to visit stmtCtx here. Also the returned error will be checked if the retry could be done in the upper layer so maybe it's enough.

store/mockstore/unistore/cophandler/cop_handler.go

TonsnakeLin · 2022-03-09T13:49:16Z

store/mockstore/unistore/tikv/dbreader/db_reader.go

 	item, err := r.txn.Get(key)
 	if err != nil && err != badger.ErrKeyNotFound {
 		return nil, errors.Trace(err)
 	}
 	if item == nil {
 		return nil, nil
 	}
+	if r.RcCheckTS {


Do we have any test case for this process?

The test in tidb_test.go would use mockstore and the path is covered there.

store/mockstore/unistore/tikv/dbreader/db_reader.go

store/mockstore/unistore/tikv/kverrors/errors.go

store/mockstore/unistore/tikv/mvcc.go

TonsnakeLin · 2022-03-10T02:50:11Z

store/mockstore/unistore/tikv/mvcc.go

+	if !isWriteLock {
+		return nil
+	}
+	return &kverrors.ErrConflict{


Whatever lock.startTS>=startTS or lock.startTS < startTS, if lock was not resloved, returns ErrConflict. Is it reasonable?
If lock was committed and lock.startTS<startTS, I think it should not return an error.

The read ts in RcCheckTS isolation level could be stale, so it could not be used to decide whether the lock could be skipped comparing ts.

cfzjywxk · 2022-03-10T09:08:35Z

/run-all-tests

cfzjywxk · 2022-03-14T02:01:19Z

@you06 @youjiali1995 @sticnarf @MyonKeminta
PTAL

you06

rest LGTM

server/conn.go

store/mockstore/unistore/tikv/dbreader/db_reader.go

store/mockstore/unistore/tikv/mvcc.go

store/mockstore/unistore/tikv/dbreader/db_reader.go

Co-authored-by: Lei Zhao <[email protected]>

cfzjywxk · 2022-03-16T10:58:56Z

/merge

cfzjywxk · 2022-03-16T11:21:03Z

/run-unit-test

cfzjywxk · 2022-03-16T11:35:52Z

/merge

bb7133

LGTM

bb7133 · 2022-03-16T11:57:12Z

/merge

cfzjywxk · 2022-03-16T12:21:41Z

/run-unit-test

cfzjywxk · 2022-03-16T12:31:29Z

/merge

ti-chi-bot · 2022-03-16T12:31:45Z

@cfzjywxk: 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.

cfzjywxk · 2022-03-16T12:34:10Z

/merge

cfzjywxk added the sig/transaction SIG:Transaction label Mar 8, 2022

cfzjywxk requested review from you06, MyonKeminta, youjiali1995 and sticnarf March 8, 2022 12:25

cfzjywxk requested a review from a team as a code owner March 8, 2022 12:25

ti-chi-bot added do-not-merge/needs-linked-issue release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 8, 2022

cfzjywxk mentioned this pull request Mar 8, 2022

txn: add document for read-consistency read tso optimization #32806

Merged

5 tasks

cfzjywxk force-pushed the rc_read_check_tso_retry branch from f9b9555 to 878eea0 Compare March 9, 2022 05:55

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 9, 2022

cfzjywxk force-pushed the rc_read_check_tso_retry branch from 878eea0 to 89e3275 Compare March 9, 2022 06:56

you06 reviewed Mar 9, 2022

View reviewed changes

TonsnakeLin reviewed Mar 9, 2022

View reviewed changes

TonsnakeLin reviewed Mar 10, 2022

View reviewed changes

cfzjywxk force-pushed the rc_read_check_tso_retry branch from 89e3275 to 8d33247 Compare March 10, 2022 08:42

cfzjywxk requested review from you06 and TonsnakeLin March 10, 2022 08:45

cfzjywxk mentioned this pull request Mar 11, 2022

txn: support read-consistency read with tso checking tikv/tikv#12097

Merged

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2022

cfzjywxk force-pushed the rc_read_check_tso_retry branch from 3aa2b4c to 21bda04 Compare March 14, 2022 02:00

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 14, 2022

cfzjywxk force-pushed the rc_read_check_tso_retry branch from 21bda04 to d7d0cb2 Compare March 14, 2022 08:56

you06 reviewed Mar 15, 2022

View reviewed changes

cfzjywxk force-pushed the rc_read_check_tso_retry branch from d7d0cb2 to 5763d32 Compare March 15, 2022 09:10

cfzjywxk and others added 4 commits March 16, 2022 18:58

Update executor/builder.go

027b638

Co-authored-by: Lei Zhao <[email protected]>

Update sessionctx/variable/session.go

009e694

Co-authored-by: Lei Zhao <[email protected]>

Update sessionctx/variable/session.go

1f9f38e

Co-authored-by: Lei Zhao <[email protected]>

Merge branch 'master' into rc_read_check_tso_retry

bf7a541

cfzjywxk changed the title ~~txn: support read consistency read with tso checking~~ txn: support read consistency read with ts checking Mar 16, 2022

cfzjywxk removed the do-not-merge/needs-linked-issue label Mar 16, 2022

bb7133 approved these changes Mar 16, 2022

View reviewed changes

Merge branch 'master' into rc_read_check_tso_retry

e572a73

ti-chi-bot added the do-not-merge/needs-linked-issue label Mar 16, 2022

cfzjywxk removed the do-not-merge/needs-linked-issue label Mar 16, 2022

ti-chi-bot added do-not-merge/needs-triage-completed do-not-merge/needs-linked-issue labels Mar 16, 2022

cfzjywxk removed do-not-merge/needs-linked-issue do-not-merge/needs-triage-completed labels Mar 16, 2022

Merge branch 'master' into rc_read_check_tso_retry

740a40d

ti-chi-bot added do-not-merge/needs-triage-completed do-not-merge/needs-linked-issue labels Mar 16, 2022

cfzjywxk removed do-not-merge/needs-linked-issue do-not-merge/needs-triage-completed labels Mar 16, 2022

ti-chi-bot merged commit d4d43ba into pingcap:master Mar 16, 2022

cfzjywxk deleted the rc_read_check_tso_retry branch March 16, 2022 12:50

TonsnakeLin mentioned this pull request Sep 14, 2022

change tidb_rc_read_check_ts to instance level #37802

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

txn: support read consistency read with ts checking #32922

txn: support read consistency read with ts checking #32922

cfzjywxk commented Mar 8, 2022 •

edited

Loading

ti-chi-bot commented Mar 8, 2022 •

edited

Loading

you06 Mar 9, 2022

cfzjywxk Mar 10, 2022

you06 Mar 9, 2022

cfzjywxk Mar 10, 2022

TonsnakeLin Mar 9, 2022

TonsnakeLin Mar 9, 2022

cfzjywxk Mar 10, 2022

TonsnakeLin Mar 9, 2022

cfzjywxk Mar 10, 2022

TonsnakeLin Mar 9, 2022

cfzjywxk Mar 10, 2022 •

edited

Loading

TonsnakeLin Mar 9, 2022

cfzjywxk Mar 10, 2022

MyonKeminta Mar 16, 2022

cfzjywxk Mar 16, 2022

TonsnakeLin Mar 9, 2022

cfzjywxk Mar 10, 2022

TonsnakeLin Mar 9, 2022

cfzjywxk Mar 10, 2022

TonsnakeLin Mar 10, 2022

cfzjywxk Mar 10, 2022

cfzjywxk commented Mar 10, 2022

cfzjywxk commented Mar 14, 2022

you06 left a comment

cfzjywxk commented Mar 16, 2022

cfzjywxk commented Mar 16, 2022

cfzjywxk commented Mar 16, 2022

bb7133 left a comment

bb7133 commented Mar 16, 2022

cfzjywxk commented Mar 16, 2022

cfzjywxk commented Mar 16, 2022

ti-chi-bot commented Mar 16, 2022

cfzjywxk commented Mar 16, 2022

txn: support read consistency read with ts checking #32922

txn: support read consistency read with ts checking #32922

Conversation

cfzjywxk commented Mar 8, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfzjywxk Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfzjywxk commented Mar 10, 2022

cfzjywxk commented Mar 14, 2022

you06 left a comment

Choose a reason for hiding this comment

cfzjywxk commented Mar 16, 2022

cfzjywxk commented Mar 16, 2022

cfzjywxk commented Mar 16, 2022

bb7133 left a comment

Choose a reason for hiding this comment

bb7133 commented Mar 16, 2022

cfzjywxk commented Mar 16, 2022

cfzjywxk commented Mar 16, 2022

ti-chi-bot commented Mar 16, 2022

cfzjywxk commented Mar 16, 2022

cfzjywxk commented Mar 8, 2022 •

edited

Loading

ti-chi-bot commented Mar 8, 2022 •

edited

Loading

cfzjywxk Mar 10, 2022 •

edited

Loading