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

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

Merged
merged 18 commits into from
Apr 6, 2022

Conversation

cfzjywxk
Copy link
Contributor

@cfzjywxk cfzjywxk commented Mar 3, 2022

What problem does this PR solve?

Issue Number: close #33159

Add design document for tso optimization for read-consistency isolation level read.

What is changed and how it works?

Check List

Side effects

Documentation

Release note

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Mar 3, 2022

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • jackysp
  • youjiali1995

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/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 3, 2022
@sre-bot
Copy link
Contributor

sre-bot commented Mar 3, 2022

If the workload is a read-heavy one or the read `qps` is large, fetching tso each time will increase the query lantecy.

The new tso itself is used to ensure the most recent data will be returned, if the data version dose not change frequently then it's unnecessary to fetch tso every time.
That is the `rc-read` could be processed in a optimistic way, the tso could be updated only when a new version data is met, then the tso cost will be saved a lot for this case.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
That is the `rc-read` could be processed in a optimistic way, the tso could be updated only when a new version data is met then the tso cost will be saved a lot for this case.
That is the `rc-read` could be processed in a optimistic way, the tso could be updated only when a new version data is met, then the tso cost will be saved a lot for this case.

## Compatibility

The default behaviours of the `read-consistency` isolation level will not change. One thing differnt is that if the user client uses `COM_STMT_FETCH` like utility to read data from `TiDB`,
there could be problem if the returned first chunk result is already used by the client but an error is reported processing next result chunk.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should elaborate on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it's needed.


## Motivation

For the read-consistency isolation level read requests in a single transaction, each will need to fetch a new `tso` to read the latest committed data.
Copy link
Contributor

Choose a reason for hiding this comment

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

tso refers to the allocator instead of a timestamp

6. The read executor in storage layer will check the read results, if more recent version does exist then a `WriteConflict` error will be returned.
7. For data write record, do check if there's new version compared with current `read_ts`.
8. For lock record, return `ErrKeyIsLocked` though the `lock.ts` is greater than the `read_ts` as the `read_ts` could be a stale one.
9. If no error is returned the query is finished. Otherwise if there's no `chunk` responsed to client yet, do retry the whole query and this time a new `for_update_ts` will be fetched.
Copy link
Contributor

Choose a reason for hiding this comment

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

What error will be returned to the client if some chunks have been responded to the client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Write conflit error will be returned to the client.

Copy link
Member

Choose a reason for hiding this comment

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

Is it acceptable to users?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In some situations the early returned result could be alredy used for example when the cursor is used by the mysql client, reporting error could not help with this. It's not recomanded to use this feature in such user case, or the init_chunk_size needs to be increased so the results are always batched in tidb-server before finishing all the checks.

## Motivation

For the read-consistency isolation level read requests in a single transaction, each will need to fetch a new `tso` to read the latest committed data.
If the workload is a read-heavy one or the read `qps` is large, fetching tso each time will increase the query lantecy.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the words such as "tso" and "qps" are in inline code format (in backquotes)?


## Motivation

For the read-consistency isolation level read requests in a single transaction, each will need to fetch a new `tso` to read the latest committed data.
Copy link
Contributor

Choose a reason for hiding this comment

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

read-consistency? is it read-committed?

Copy link
Contributor

Choose a reason for hiding this comment

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

It actually refers to the term "statement-level read consistency" from Oracle. Yes, seems better to clarify it's the semantics of "read committed" in TiDB.


## Motivation

For the read-consistency isolation level read requests in a single transaction, each will need to fetch a new `ts` to read the latest committed data.
Copy link
Contributor

Choose a reason for hiding this comment

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

read-consistency? is it a read-committed isolation?

@TonsnakeLin
Copy link
Contributor

I think the proposal maybe make qps jitter except the work-load is a very very heavy read service

6. The read executor in storage layer will check the read results, if more recent version does exist then a `WriteConflict` error will be returned.
7. For data write record, do check if there's new version compared with current `read_ts`.
8. For lock record, return `ErrKeyIsLocked` though the `lock.ts` is greater than the `read_ts` as the `read_ts` could be a stale one.
9. If no error is returned the query is finished. Otherwise if there's no `chunk` responsed to client yet, do retry the whole query and this time a new `for_update_ts` will be fetched.
Copy link
Member

Choose a reason for hiding this comment

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

I think since tidb has to read it twice, why not read it the first time with maxint64, and then, calculate the tso brought back in the read response as a max+1 and read it again. This should avoid getting tso in RC?

Copy link
Contributor

Choose a reason for hiding this comment

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

The timestamp in the read response is not certainly new enough. It is possible that there is a write record with larger commit TS in the third shard. And if the error is caused by a lock, we actually cannot know its commit TS.

Getting a new ts from TSO works simple and correct.

The only thing I might do different here is to get ts parallelly with the first read. I believe it won't increase much load thanks to the tso batching.

Copy link
Contributor

@youjiali1995 youjiali1995 left a comment

Choose a reason for hiding this comment

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

rest LGTM

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 22, 2022
Copy link
Member

@jackysp jackysp 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 status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Apr 6, 2022
@jackysp
Copy link
Member

jackysp commented Apr 6, 2022

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: eca7a68

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 6, 2022
@ti-chi-bot ti-chi-bot merged commit 37d86da into master Apr 6, 2022
@ti-chi-bot ti-chi-bot deleted the cfzjywxk-patch-1 branch April 6, 2022 10:18
@311ybb
Copy link

311ybb commented May 14, 2022

  1. If the query is executed first time, do not fetch a new for_update_ts and just use the last valid one(start_ts for the first time).
    Q: what is last valid? the for_update_ts for last SQL?

2.The read executor in the storage layer will check the read results, if a more recent version does exist then a WriteConflict error is reported.
Q: How executor check if there is a new version? if more check will impact performance?

@311ybb 311ybb mentioned this pull request May 14, 2022
@cfzjywxk
Copy link
Contributor Author

  1. If the query is executed first time, do not fetch a new for_update_ts and just use the last valid one(start_ts for the first time).
    Q: what is last valid? the for_update_ts for last SQL?

2.The read executor in the storage layer will check the read results, if a more recent version does exist then a WriteConflict error is reported. Q: How executor check if there is a new version? if more check will impact performance?

@311ybb

  1. Yes, the last used for_update_ts or the start_ts could be used.
  2. The executor would check if there's a more recent version than the read ts, this impact on performance may not be much in most situations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/docs release-note-none Denotes a PR that doesn't merit a release note. sig/transaction SIG:Transaction size/M Denotes a PR that changes 30-99 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.

txn: support tso optimization for read-consistency isolation level read.
10 participants