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

*: (DRAFT) Refactor stale read code #32325

Closed
wants to merge 15 commits into from

Conversation

lcwangchao
Copy link
Collaborator

@lcwangchao lcwangchao commented Feb 14, 2022

This is a draft to refactor stale read according to new txn context management design: #30535 . In this PR, the stale read implements are moved to a standalone package to make the code more cohesive. This PR also provides some interfaces and decouple the stale read processing to some inherent classes according the different scenes.

staleReadTxnContextProvider

In this PR we introduce a new struct staleReadTxnContextProvider. staleReadTxnContextProvider implements TxnContextProvider , and it provides the txn context when the current scene is stale read. staleReadTxnContextProvider implements the below methods:

  • GetTxnInfoSchema returns the information schema for stale read.
  • GetReadTS returns the ts for read
  • GetReadReplicaScope returns the read replica prefer for the stale read

Notice that staleReadTxnContextProvider is only one of subclasses for TxnContextProvider , we'll implement other providers like RRTxnContextProvider or RCTxnContextProvider . So the concept of these methods should not limited only to stale read.

After moving the stale read context to TxnManager we can the fetch these context from session instead of passing them as method arguments, it makes the code more simple and clear.

staleread.StmtProcessor

In this PR, we moved the stale read processing code to staleread.StmtProcessor . Comparing the old code, it decoupled stale read processing from preprocessor and make the code easier to do uint test.

We should process stale read for below scenes:

  • Initialize stale read context before execute the sql.
  • Prepare a sql and extracting the stale read timestamp which should be saved in prepared statement
  • Validating a sql in create view (Create view should fail when the select statement contains stale read)

The process rules for the above scenes are not the same. The old implement write all the processing code together that makes it hard to understand and easier to get a bug. In the PR, we separate them into different logicals. The below processors are introduced:

  • InitTxnContextProcessor It is used to initialize stale read txn context. This processor will parse the stale read context from sql and sys variables and set the TxnManager's provider to staleReadTxnContextProvider
  • PrepareParseProcessor It is used to parse the stale read ts for a sql to prepare. It will not affect the current txn context.
  • CreateViewProcessor It is used to forbid stale read sql when creating a view.

All the above processors will parse the stale read ts, but the paring ways may be a little different. For example CreateViewProcessor will ignore sys variables and only check stale read settings in sql.

All the above processors will implement a interface below and the preprocessor need not to care about the differences for the above scenes:

type StmtProcessor interface {
	// GetType returns the process type
	GetType() ProcessType
	// IsStmtStaleness indicates that whether the stmt has the staleness declaration
	IsStmtStaleness() bool
	// GetStalenessInfoSchema returns the information schema if it is stale read, otherwise returns nil
	GetStalenessInfoSchema() infoschema.InfoSchema
	// GetStalenessReadTS returns the ts if it is stale read, otherwise returns 0
	GetStalenessReadTS() uint64

	// OnSelectTable will be called when process table in select statement.
	// Support type: ProcessTypeInitTxnContext, ProcessTypeParsePrepare, ProcessTypeValidateCreateView
	OnSelectTable(tn *ast.TableName) error
	// OnExecuteStmtWithPreparedTS will be called when process execute statement
	// Support type: ProcessTypeInitTxnContext
	OnExecuteStmtWithPreparedTS(evaluator PreparedTSEvaluator) error
	// OnBeginStmt will be called when process begin statement
	// Support type: ProcessTypeInitTxnContext
	OnBeginStmt(begin *ast.BeginStmt) error
}

Other

In this PR, the staleread package also provides some methods to encapsulate some stale read operations. For example: staleread.IsStaleness returns wether the current statement is staleness according to the current txn provider and ExplicitStartStaleReadTxn starts new stale read txn. To move most of the complexity to the staleread package, the code is easier to maintain.

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

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.

@ti-chi-bot ti-chi-bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 14, 2022
@ti-chi-bot
Copy link
Member

@lcwangchao: PR needs rebase.

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/test-infra repository.

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2022
@lcwangchao lcwangchao changed the title *: Refactor stale read code *: (DRAFT) Refactor stale read code Feb 17, 2022
distsql/request_builder.go Outdated Show resolved Hide resolved
Comment on lines +30 to +33
"github.com/pingcap/tidb/kv"

"github.com/pingcap/tidb/sessiontxn"

Copy link
Member

Choose a reason for hiding this comment

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

Please format this part.

@ti-chi-bot
Copy link
Member

[FORMAT CHECKER NOTIFICATION]

Notice: To remove the do-not-merge/needs-linked-issue label, please provide the linked issue number on one line in the PR body, for example: Issue Number: close #123 or Issue Number: ref #456.

📖 For more info, you can check the "Contribute Code" section in the development guide.

@lcwangchao lcwangchao closed this Apr 15, 2022
@lcwangchao lcwangchao deleted the staleread1 branch July 5, 2022 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/needs-linked-issue needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants