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

pessimistic-txn: solve non-pessimistic-lock conflict #4801

Merged

Conversation

@youjiali1995
Copy link
Contributor

youjiali1995 commented May 29, 2019

What have you changed? (mandatory)

  • Upgrade to the latest kvproto, which added for_update_ts field to prewrite request.
  • Record for_update_ts in Lock, and judge whether a transaction is optimistic or pessimistic by it.
  • Add for_update_ts field to Options to avoid changing prewrite interface, and change acquire_pessimistic_lock interface to adapt it.
  • Overwrite optimistic lock in pessimistic_prewrite if request's for_update_ts is greater than lock's for_update_ts.

What are the type of the changes? (mandatory)

  • Bug fix (change which fixes an issue)

How has this PR been tested? (mandatory)

Add a unit test to test the logic and there is no PessimisticLockNotFound error when sysbenching.

Does this PR affect documentation (docs) or release note? (mandatory)

No

Does this PR affect tidb-ansible update? (mandatory)

No

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
request's for_update_ts is greater than lock's for_update_ts

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
@youjiali1995

This comment has been minimized.

Copy link
Contributor Author

youjiali1995 commented May 29, 2019

@BusyJay

This comment has been minimized.

Copy link
Contributor

BusyJay commented May 29, 2019

Horrible pull request title!

@youjiali1995 youjiali1995 changed the title pessimistic-txn: overwrite optimistic lock in pessimistic_prewrite if request's for_update_ts is greater than lock's for_update_ts pessimistic-txn: solve optimistic lock conflict May 29, 2019
@youjiali1995 youjiali1995 added the C: Txn label May 29, 2019
src/storage/mvcc/txn.rs Outdated Show resolved Hide resolved
src/storage/mvcc/txn.rs Outdated Show resolved Hide resolved
src/storage/mvcc/txn.rs Outdated Show resolved Hide resolved
Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
// Duplicated command. No need to overwrite the lock and data.
MVCC_DUPLICATE_CMD_COUNTER_VEC.prewrite.inc();
return Ok(());
// Abort on lock belonging to other transaction if

This comment has been minimized.

Copy link
@coocood

coocood May 29, 2019

Contributor

This block is too long, extract a function is better.

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 May 29, 2019

Author Contributor

I think now it is clearer than extracting a function, and the function needs many parameters.

This comment has been minimized.

Copy link
@coocood

coocood May 29, 2019

Contributor

Only 4 parameters: self, lock, key, for_update_ts.

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 May 29, 2019

Author Contributor

And also is_pessimistic_lock. The name of function is redundent, like check_pessimistic_prewrite_lock_conflict.

This comment has been minimized.

Copy link
@coocood

coocood May 29, 2019

Contributor

OK, then we can extract the lines after if is_pessimistic_lock block to a function.

This comment has been minimized.

Copy link
@coocood

coocood May 29, 2019

Contributor

The name can be check_non_pessimistic_lock_conflict

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 May 29, 2019

Author Contributor

ok

…prewrite-conflict

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
@@ -327,14 +336,15 @@ impl<S: Snapshot> MvccTxn<S> {
key: key.into_raw()?,
});
}

// No need to check data constraint, it's resolved by pessimistic locks.
self.put_lock(

This comment has been minimized.

Copy link
@coocood

coocood May 29, 2019

Contributor

we can reduce 2 parameters for this function.

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 May 29, 2019

Author Contributor

yes

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
@youjiali1995 youjiali1995 changed the title pessimistic-txn: solve optimistic lock conflict pessimistic-txn: solve non-pessimistic-lock conflict May 29, 2019
Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
…iali1995/tikv into solve-pessimistic-prewrite-conflict
value: Option<Value>,
is_pessimistic_txn: bool,
txn_size: u64,
options: &Options,
) {
if value.is_none() || is_short_value(value.as_ref().unwrap()) {
self.lock_key(

This comment has been minimized.

Copy link
@coocood

coocood May 29, 2019

Contributor

lock_key can reduce parameters too.

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 May 29, 2019

Author Contributor

😅my fault.

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
…iali1995/tikv into solve-pessimistic-prewrite-conflict
@coocood

This comment has been minimized.

Copy link
Contributor

coocood commented May 29, 2019

LGTM

@@ -49,7 +49,7 @@ pub type Callback<T> = Box<dyn FnOnce(Result<T>) + Send>;
// Short value max len must <= 255.
pub const SHORT_VALUE_MAX_LEN: usize = 64;
pub const SHORT_VALUE_PREFIX: u8 = b'v';
pub const PESSIMISTIC_TXN: u8 = b'p';
pub const FOR_UPDATE_TS_PREFIX: u8 = b'f';

This comment has been minimized.

Copy link
@AndreMouche

AndreMouche May 30, 2019

Member

I think it's not a good idea to change the prefix .. we will have two different kinds of prefix between versions.

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 May 30, 2019

Author Contributor

Even if the prefix is the same, different versions won't be compatible because the old version doesn't record for_update_ts after the prefix.

This comment has been minimized.

Copy link
@zhangjinpeng1987

zhangjinpeng1987 Jun 5, 2019

Member

I think it is ok since this feature is in developing.

@@ -1322,6 +1322,7 @@ fn future_prewrite<E: Engine>(
let mut options = Options::default();
options.lock_ttl = req.get_lock_ttl();
options.skip_constraint_check = req.get_skip_constraint_check();
options.for_update_ts = req.get_for_update_ts();

This comment has been minimized.

Copy link
@AndreMouche

AndreMouche May 30, 2019

Member

Why we need for_update_ts in prewrite, seems we already have it by acquire_pessimistic_lock?

This comment has been minimized.

Copy link
@AndreMouche

AndreMouche May 30, 2019

Member

By the way, why the for_update_ts is the same for all key in the prewrite command?

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 May 30, 2019

Author Contributor

for_update_ts is used to resolve non-pessimistic-lock conflict.

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 May 30, 2019

Author Contributor

TiDB uses the last for_update_ts as prewrite's.

// is replaced atomicly.
//
// Optimistic lock's for_update_ts is its start_ts.
let lock_for_update_ts = if lock.for_update_ts > 0 {

This comment has been minimized.

Copy link
@AndreMouche

AndreMouche May 30, 2019

Member

When will it happen? Here we meet a pessimistic lock which does not belong to current transaction when we prewrite, which I think this shouldn't happen.

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 May 30, 2019

Author Contributor

It is a non-pessimistic lock. The google doc has more info.


self.check_non_pessimistic_lock_conflict(options.for_update_ts, &lock)?;
} else {
if lock.lock_type != LockType::Pessimistic {

This comment has been minimized.

Copy link
@AndreMouche

AndreMouche May 30, 2019

Member

How about using else if directly?

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
…iali1995/tikv into solve-pessimistic-prewrite-conflict
@youjiali1995 youjiali1995 added the S: DNM label May 30, 2019
Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
…iali1995/tikv into solve-pessimistic-prewrite-conflict
@youjiali1995 youjiali1995 removed the S: DNM label May 31, 2019
@youjiali1995

This comment has been minimized.

Copy link
Contributor Author

youjiali1995 commented May 31, 2019

@@ -143,7 +143,7 @@ impl<S: Snapshot> MvccTxn<S> {

// If the value is short, lock key and put value.
// If not, lock key.
fn put_lock(
fn lock_key_and_put_value_if_short(

This comment has been minimized.

Copy link
@coocood

coocood May 31, 2019

Contributor

The logic is lock_key_and_put_value_if_long.
And I think the name is too complex.
How about just prewrite_key or prewrite_key_value?

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 May 31, 2019

Author Contributor

Yes. I prefer prewrite_key_value

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
…iali1995/tikv into solve-pessimistic-prewrite-conflict
@coocood

This comment has been minimized.

Copy link
Contributor

coocood commented May 31, 2019

LGTM

@youjiali1995

This comment has been minimized.

Copy link
Contributor Author

youjiali1995 commented Jun 4, 2019

..
} => write!(
f,
"kv::command::acquirepessimisticlock keys({}) @ {},{} | {:?}",
"kv::command::acquirepessimisticlock keys({}) @ {} | {:?}",

This comment has been minimized.

Copy link
@zhangjinpeng1987

zhangjinpeng1987 Jun 5, 2019

Member

Better to display for_update_ts, it is useful for debugging.

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 Jun 5, 2019

Author Contributor

Done

@@ -199,14 +178,40 @@ impl<S: Snapshot> MvccTxn<S> {
Ok(())
}

fn handle_non_pessimistic_lock_conflict(
&self,

This comment has been minimized.

Copy link
@zhangjinpeng1987

zhangjinpeng1987 Jun 5, 2019

Member

Seems this function never use self.

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 Jun 5, 2019

Author Contributor

It is a bit complex to call it without self.

youjiali1995 added 2 commits Jun 5, 2019
Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
Copy link
Member

zhangjinpeng1987 left a comment

LGTM

@youjiali1995 youjiali1995 added the S: LGT1 label Jun 6, 2019
primary: lock.primary,
ts: lock.ts,
// Set ttl to 0 so TiDB will resolve lock immediately.
ttl: 0,

This comment has been minimized.

Copy link
@AndreMouche

AndreMouche Jun 6, 2019

Member

what if this lock is a normal primary key, and the corresponding txn is not time out?

@youjiali1995 youjiali1995 merged commit ae73f6c into tikv:master Jun 6, 2019
2 checks passed
2 checks passed
DCO All commits are signed off!
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
@youjiali1995 youjiali1995 deleted the youjiali1995:solve-pessimistic-prewrite-conflict branch Jun 6, 2019
sticnarf added a commit to sticnarf/tikv that referenced this pull request Jun 10, 2019
Signed-off-by: Yilin Chen <sticnarf@gmail.com>

Makefile: make sure gdb is installed before sse4.2 check (tikv#4832)

Signed-off-by: Kaige Ye <ye@kaige.org>

Upgrade sys-info (tikv#4760)

* *: upgrade sys-info crate

This fixes a problem with the next toolchain upgrade
where rust fails to link the native components of the crate.

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* *: bump sys-info to 0.5.7

Signed-off-by: Brian Anderson <andersrb@gmail.com>

Batch Top N Executor (tikv#4825)

Signed-off-by: Breezewish <breezewish@pingcap.com>

Add help message in doc:go-client-api.md (tikv#4763)

* add help message in doc:go-client-api.md

Signed-off-by: yy <cacheyy@qq.com>

* update go-client-api.md

Signed-off-by: yy <cacheyy@qq.com>

Modify Makefile to distinguish between developer and packaging use cases (tikv#4687)

* make: Add new "dist_release" rules

To make the optimized build faster the existing "release" rules are going to
changed such that they are not identical to the actual releases. Primarily they
will not have debuginfo by default and will use thinLTO instead of LTO.

This adds new "dist_release", etc rules for the CI/CD system to use.

For now they are identical to the existing rules. After CI is updated
the "release" rules will be changed.

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* make: Document release rules

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Makefile: indicate use of fail_release

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Clarify the distinction in instruction set for release targets

Signed-off-by: Brian Anderson <andersrb@gmail.com>

Makefile: fix gdb check (tikv#4840)

Signed-off-by: Kaige Ye <ye@kaige.org>

pessimistic-txn: solve non-pessimistic-lock conflict (tikv#4801)

* txn: replace is_pessimistic_lock to for_update_ts in Lock

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* pessimistic-txn: overwrite optimistic lock in pessimistic_prewrite if
request's for_update_ts is greater than lock's for_update_ts

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* modify comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* address comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* address comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* address comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* address comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* add comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* return Error let TiDB to resolve lock

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* address comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* address comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

coprocessor: add batch aggregate function BitAnd/BitOr/BitXor (tikv#4824)

Batch Top N Layered Benchmarks (tikv#4827)

* Add Top N benchmarks

Signed-off-by: Breezewish <breezewish@pingcap.com>

* Address some comments in previous PRs

Signed-off-by: Breezewish <breezewish@pingcap.com>

coprocessor: add batch aggregate function Max/Min (tikv#4837)

Implement RpnFunction MultiplyDecimal (tikv#4849)

Signed-off-by: Breezewish <breezewish@pingcap.com>

Add missing fsync calls in the snapshot module (tikv#4850)

Signed-off-by: Ben Pig Chu <benpichu@gmail.com>

use HTTP to enable jemalloc profile (tikv#4600)

use HTTP to enable jemalloc profile

Signed-off-by: Yang Keao <keao.yang@yahoo.com>

coprocessor: use servo_arc in BatchTopNExecutor (tikv#4854)

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

Fix clippy warnings

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

Fix test

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

Add docs to function.rs

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

Add example output of the macro in the test of the macro.

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

fix broken url for configuration options (tikv#4856)

Signed-off-by: Yukang <moorekang@gmail.com>

shrink the latch waiting list (tikv#4844)

Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>

Fix clippy

Signed-off-by: Yilin Chen <sticnarf@gmail.com>

scheduler use spin::Mutex (tikv#4829)

* scheduler use spinlock

Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>

Better panic info

Signed-off-by: Yilin Chen <sticnarf@gmail.com>
youjiali1995 added a commit to youjiali1995/tikv that referenced this pull request Jun 13, 2019
* txn: replace is_pessimistic_lock to for_update_ts in Lock

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* pessimistic-txn: overwrite optimistic lock in pessimistic_prewrite if
request's for_update_ts is greater than lock's for_update_ts

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* modify comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* address comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* address comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* address comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* address comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* add comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* return Error let TiDB to resolve lock

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* address comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* address comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
sticnarf pushed a commit to sticnarf/tikv that referenced this pull request Oct 27, 2019
* txn: replace is_pessimistic_lock to for_update_ts in Lock

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* pessimistic-txn: overwrite optimistic lock in pessimistic_prewrite if
request's for_update_ts is greater than lock's for_update_ts

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* modify comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* address comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* address comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* address comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* address comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* add comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* return Error let TiDB to resolve lock

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* address comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>

* address comment

Signed-off-by: youjiali1995 <zlwgx1023@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.