*: stale reads compatible with prepare by xhebox · Pull Request #25156 · pingcap/tidb

xhebox · 2021-06-04T08:45:21Z

What problem does this PR solve?

Issue Number: close #24932

Problem Summary: Prepare is now compatible with stale reads.

What is changed and how it works?

What's Changed:

snapshotTS != 0 is used to indicate that it is a stale read.

CachedPrepareStmt has a new field SnapshotTS to store the result of preprocessing in PrepareExec.
newExecutorBuilder has a new parameter, snapshotTS. It is passed from session.ExecutePreparedStmt, along cachedPlanExec or CompileExecutePreparedStmt. The one in session comes from CachedPrepareStmt.
Skip infoschema check in IsCachedExecOk and OptimizePreparedPlan, if snapshotTS != 0.
plannercore.Execute plan has a new field SnapshotTS to store the timestamp from CachedPrepareStmt.
executorExec.buildExecute will assign snapshotTS from plannercore.Execute plan to executorBuilder.

1-3 covered the prepared execution directly from session/mysql-protocol, and 4-5 covered the execution from plain SQL.

NEW CHANGES: to support dynamic timestamp like NOW() - INTERVAL 1 SECOND:

CachedPrepareStmt do not store infoschema anymore. You need to recheck infoschema before execute to ensure consistency anyway.
As Of statements evaluation is stored/prepared as a closure to support dynamic re-evaluation. For non-prepare statements, they can use LastSnapshotTS to avoid re-evaluation.

Check List

Tests

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

Release note

Support prepare stale reads SQL

nolouch · 2021-06-04T09:32:27Z

What will happen in this situation?

create table t (id int) insert into t values (1) ---- 1s prepare s from select * from t as of timestamp Now() - INTERVAL 2 Second -----2s execute s ; ----- 4s alter table t add column age int; ----- 5s insert into t values (2, 2) --------5s execute s; ----- 8s

maybe we need to store the expression rather than SanpshotTS.

I wanted to prepare the handle of infoschema in #25080, but that is not true anymore. The handle of infoschema is now added.

I want to get the result (2, 2) in the time point 8s. but I guess you implement will always get the result 1.

Yeah, the timestamp is fixed once preprocessed now.

Your expectation needs a re-preprocessing, which drops all the prepared efforts. It is not a simple reason of my implementation. If timestamp is changed, schema and snapshot is changed, the result from previous prepare statements is meaningless.

Its semantic may be intuitive, but it does not make much sense under our current framework of prepare statements.

A full support, I guess, will need to store the expr of as of into the plan. And the optimizer should consider infoschema and snapshot in their optimization process. Then it will be much easier to implement.

I could provide a hack solution, if you really want it to be true.

No, it must be re-preprocessed. Infoschema may change. Even if it is not changed, a check for stale infoschema is needed, which leads to one more tikv metadata access.

Otherwise, the result is inconsistent.

Agree with you.

Glad to hear that but it can not be true for now. I can only hack around. The solution will be tricky.

Yes, I mean I can accept re-preprocessed, cannot accept the result is fixed on the point in mine example. BTW, Even if we have the infoschema cache, still we need to access TiKV to get the schema version for every request?

No for the current solution. Because it is already ensured when prepare. If you want to ensure the consistency for NOW() - 2 seconds, however, yes, you need extra request. You don't know the schema version before 2 seconds. It is not prepared due to NOW(). If you don't check, it may be outdated.

FYI, check

tidb/domain/domain.go

Lines 103 to 107 in 5c95062

m := meta.NewSnapshotMeta(snapshot)

neededSchemaVersion, err := m.GetSchemaVersion()

if err != nil {

return nil, false, 0, nil, err

}

for the code of getting SchemaVersion.

I have checked it, the implementation is different from what I understand. I think the schema version is guaranteed to increase, it could be optimized to reduce the extra request by check the schema version is not changed between the older request and the up-to-date schema version.

Otherwise, it is easy to have a single point of bottleneck with the current implementation, because the meta information is always read from a certain TiKV node.

xhebox · 2021-06-04T10:12:23Z

While the change of infoschema for stale reads are expected, you may argue that preparedStmtExec does not have such logic, i.e. change infoschema for updateRead.

But it does. It will eventually invoke planner.Optimize, which does get the latest infoschema for preparedStmt.ForUpdateRead. It is located at https://github.com/pingcap/tidb/blob/master/planner/optimize.go#L240-L250.

nolouch · 2021-06-07T07:27:58Z

PTAL @JmPotato @Yisaer

JmPotato

Except for the problem @nolouch said, the rest LGTM.

xhebox · 2021-06-07T14:49:48Z

a.ExplicitStaleness is added as a parameter, found #25206 lost the point get case, just like me.

xhebox · 2021-06-07T14:51:57Z

Moved to handleAsOfAndReadTS to center the initialize of LastSnapshotTS and other parameters.

ti-chi-bot · 2021-06-10T08:00:27Z

@JmPotato: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

Details

In response to this:

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 · 2021-06-11T09:23:51Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JmPotato
nolouch

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.

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

nolouch · 2021-06-11T09:25:00Z

/merge

ti-chi-bot · 2021-06-11T09:25:04Z

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

Details

Commit hash: 703a67d

nolouch · 2021-06-11T09:42:57Z

/run-unit-test
/run-check_dev_2

nolouch · 2021-06-11T11:26:59Z

/run-integration-common-test

Yisaer · 2021-06-11T11:25:23Z

+	tk.MustExec(fmt.Sprintf(`start transaction read only as of timestamp '%s'`, time2.Format("2006-1-2 15:04:05.000")))
+	c.Assert(tk.ExecToErr(staleSQL), NotNil)
+	tk.MustExec("commit")
+
+	// test prepared stale select in stale txn
+	tk.MustExec(fmt.Sprintf(`start transaction read only as of timestamp '%s'`, time2.Format("2006-1-2 15:04:05.000")))
+	tk.MustQuery("execute s").Check(staleRows)
+	tk.MustExec("commit")


Is execute s expected to be executed successfully in staleness transaction while the staleSQL are not?

How about not in prepare?

Yes, it is expected. And it should be possible to throw an error in OptimizePreparedPlan.

IMO this is not expected. We should avoid multiple snapshotTS in a transaction as users may think it's data inconsistency error during transaction.

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2021-06-11T11:33:10Z

cherry pick to release-5.1 in PR #25371

Yisaer · 2021-06-11T12:12:53Z

+// handleAsOfAndReadTS tries to handle as of closure, or possibly read_ts.
+// If read_ts is not nil, it will be consumed.
+// If as of is not nil, timestamp is used to get the history infoschema from the infocache.
+func (p *preprocessor) handleAsOfAndReadTS(node *ast.AsOfClause) {


I think handleAsOfAndReadTS is much complex now. We need to add example to explain it and add unit test to ensured logic.

xhebox requested review from a team as code owners June 4, 2021 08:45

xhebox requested review from qw4990 and removed request for a team June 4, 2021 08:45

ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 4, 2021

xhebox force-pushed the stale_3 branch 2 times, most recently from 05ebf02 to 5ba47c9 Compare June 4, 2021 09:06

github-actions Bot added sig/execution SIG execution sig/sql-infra SIG: SQL Infra labels Jun 4, 2021

nolouch reviewed Jun 4, 2021

View reviewed changes

xhebox commented Jun 4, 2021

View reviewed changes

xhebox force-pushed the stale_3 branch from 5ea1c25 to 511876e Compare June 4, 2021 10:17

xhebox mentioned this pull request Jun 4, 2021

stale read like 'select ... as of timestamp ...' for point get not work' #25167

Closed

xhebox commented Jun 4, 2021

View reviewed changes

Comment thread executor/adapter.go Outdated

xhebox requested a review from nolouch June 4, 2021 10:39

xhebox force-pushed the stale_3 branch from 5d0cbb1 to e4193b5 Compare June 4, 2021 11:33

nolouch added the needs-cherry-pick-release-5.1 label Jun 7, 2021

JmPotato reviewed Jun 7, 2021

View reviewed changes

Comment thread planner/core/common_plans.go Outdated

JmPotato reviewed Jun 7, 2021

View reviewed changes

xhebox force-pushed the stale_3 branch from 3d83f84 to ee13e1f Compare June 7, 2021 14:41

ti-chi-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 7, 2021

xhebox commented Jun 7, 2021

View reviewed changes

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

JmPotato reviewed Jun 8, 2021

View reviewed changes

Comment thread executor/stale_txn_test.go Outdated

JmPotato mentioned this pull request Jun 8, 2021

support stale-read testcase pingcap/tipocket#416

Open

nolouch reviewed Jun 9, 2021

View reviewed changes

Comment thread planner/core/preprocess.go Outdated

JmPotato approved these changes Jun 10, 2021

View reviewed changes

nolouch removed sig/execution SIG execution sig/sql-infra SIG: SQL Infra labels Jun 11, 2021

JmPotato approved these changes Jun 11, 2021

View reviewed changes

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 Jun 11, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 11, 2021

Merge branch 'master' into stale_3

ab468f3

github-actions Bot added sig/execution SIG execution sig/sql-infra SIG: SQL Infra labels Jun 11, 2021

Merge branch 'master' into stale_3

b6d8140

Yisaer reviewed Jun 11, 2021

View reviewed changes

ti-chi-bot merged commit 9189ec6 into pingcap:master Jun 11, 2021

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jun 11, 2021

cherry pick pingcap#25156 to release-5.1

567b378

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Jun 11, 2021

*: stale reads compatible with prepare (#25156) #25371

Merged

Yisaer reviewed Jun 11, 2021

View reviewed changes

xhebox deleted the stale_3 branch June 11, 2021 12:18

xhebox mentioned this pull request Jun 11, 2021

planner: forbid prepared stale select in txn #25375

Merged

ti-chi-bot pushed a commit that referenced this pull request Jun 15, 2021

*: stale reads compatible with prepare (#25156) (#25371)

270a4b2

ti-srebot mentioned this pull request Jun 15, 2021

planner: forbid prepared stale select in txn (#25375) #25413

Merged

lcwangchao mentioned this pull request Jun 28, 2021

Maybe we should rename 'ExplicitStaleness' to 'IsStaleness' #25790

Closed

MyonKeminta mentioned this pull request Jul 8, 2021

session: Fix txn.onStmtEnd is not called when some special error occurs in ExecutePreparedStmt #26056

Merged

ti-srebot mentioned this pull request Jul 9, 2021

session: Fix txn.onStmtEnd is not called when some special error occurs in ExecutePreparedStmt (#26056) #26080

Merged

	m := meta.NewSnapshotMeta(snapshot)
	neededSchemaVersion, err := m.GetSchemaVersion()
	if err != nil {
	return nil, false, 0, nil, err
	}

Conversation

xhebox commented Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

nolouch Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nolouch Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xhebox Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xhebox Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nolouch Jun 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xhebox Jun 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nolouch Jun 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xhebox Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nolouch commented Jun 7, 2021

Uh oh!

Uh oh!

JmPotato left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ti-chi-bot commented Jun 10, 2021

Uh oh!

ti-chi-bot commented Jun 11, 2021

Uh oh!

nolouch commented Jun 11, 2021

Uh oh!

ti-chi-bot commented Jun 11, 2021

Uh oh!

nolouch commented Jun 11, 2021

Uh oh!

nolouch commented Jun 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xhebox Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

xhebox commented Jun 4, 2021 •

edited

Loading

nolouch Jun 4, 2021 •

edited

Loading

nolouch Jun 4, 2021 •

edited

Loading

xhebox Jun 4, 2021 •

edited

Loading

xhebox Jun 4, 2021 •

edited

Loading

nolouch Jun 7, 2021 •

edited

Loading

xhebox Jun 7, 2021 •

edited

Loading

nolouch Jun 7, 2021 •

edited

Loading

xhebox Jun 4, 2021 •

edited

Loading

xhebox Jun 11, 2021 •

edited

Loading