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.

Sign up for GitHub

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

Jump to bottom

cop: ignore locks with the same start_ts #52461

Merged
merged 2 commits into from
Apr 11, 2024
Merged

cop: ignore locks with the same start_ts #52461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c46a40e
fix: put start_ts to resolved_locks for normal coprocessor reads
ekexium Apr 9, 2024
22af508
fix: resolved_locks in unistore.cophandler
ekexium Apr 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 11 additions & 0 deletions pkg/store/copr/coprocessor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,17 @@ func (c *CopClient) BuildCopIterator(ctx context.Context, req *kv.Request, vars
buildTaskElapsed: *buildOpt.elapsed,
runawayChecker: req.RunawayChecker,
}
// Pipelined-dml can flush locks when it is still reading.
// The coprocessor of the txn should not be blocked by itself.
// It should be the only case where a coprocessor can read locks of the same ts.
//
// But when start_ts is not obtained from PD,
// the start_ts could conflict with another pipelined-txn's start_ts.
// in which case the locks of same ts cannot be ignored.
// We rely on the assumption: start_ts is not from PD => this is a stale read.
if !req.IsStaleness {
Copy link
Contributor

Choose a reason for hiding this comment

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

For tidb_snapshot read path, the staleness may not be set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit confusing. Is it intentional or an oversight? I suppose that tidb_snapshot is solely used for stale reads.

Copy link
Contributor

@cfzjywxk cfzjywxk Apr 10, 2024 •
edited

Choose a reason for hiding this comment

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

The stale flag is not set in the kv request for tidb_snapshot if I do not remember wrong, there are two stale read implementaions in tidb and tidb_snasphot is the earlier one.

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 I've verified that. I mean if it's unintentional we can fix it by setting IsStaleness 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.

Do you mean setting IsStaleness for tidb_snaoshot read?

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

Copy link
Contributor

Choose a reason for hiding this comment

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

In theory, this is the way to proceed with the change.
However, it's a breaking change because tidb_snapshot wasn't set before (setting it will trigger a check on safe_ts). Currently, it's unclear if other components rely on this behavior.
We can proceed by merging this PR first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then it's possible that when tidb_snapshot is set to a ts conflicting with a pipelined txn, coprocessor ignores the locks while point_get doens't?

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 so though tidb_snapshot seems not to be used widely currenly.

it.resolvedLocks.Put(req.StartTs)
}
it.tasks = tasks
if it.concurrency > len(tasks) {
it.concurrency = len(tasks)
Expand Down
2 changes: 1 addition & 1 deletion pkg/store/mockstore/unistore/cophandler/closure_exec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1185,7 +1185,7 @@ func safeCopy(b []byte) []byte {
}

func checkLock(lock mvcc.Lock, key []byte, startTS uint64, resolved []uint64) error {
if isResolved(startTS, resolved) {
if isResolved(lock.StartTS, resolved) {
return nil
}
lockVisible := lock.StartTS < startTS
Expand Down