Add for_update_ts check to PrewriteRequest

[MyonKeminta]

Ref: tikv/tikv#14311

Naming suggestions are welcome...

[ekexium]

Maybe it would be better to rename check to constraint?
I'm not quite sure if allowing for_update_ts < expected is a good idea. It makes things a bit difficult to reason about. If we do, then the name could be something like max_allowed... or ...upper_bound

[MyonKeminta]

Maybe it would be better to rename check to constraint? I'm not quite sure if allowing for_update_ts < expected is a good idea. It makes things a bit difficult to reason about. If we do, then the name could be something like max_allowed... or ...upper_bound

The reason of allowing "<" is that it can guarantee there are no not-detected write conflict. The detail was discussed in the issue tikv/tikv#14311 . Does that make sense to you?

[ekexium]

Yeah I've checked that.
There is a loss of pessimistic lock in the example you gave, so it is acceptable to return a PesLockNotFound error.
IMO if simply rejecting for_update_ts < expected cannot cause correctness problems and is much unlikely to happen (at least in normal environmetns), but can reduce the cognitive load (for everyone reading this piece of code and wondering the same question), then it is worth considering.

[MyonKeminta]

@cfzjywxk PTAL, how's your opinion?

[cfzjywxk]

Permitting for_update_ts < expected creates confusion and requires additional effort to ensure its accuracy. Perhaps we could implement an exact check and reject the replayed pessimistic lock immediately, as this occurrence is infrequent.

If testing reveals scenarios that were not previously considered and may result in a significant number of replayed pessimistic locks, maybe we could take them into further consideration.

[cfzjywxk]

As the fair lock mode supports only single key lock now, does it mean for a transaction the maximum length of this array is txn.total_statment_number?

[MyonKeminta]

No. It's possible that a statement calls LockKeys more than once and locks one key in each call.

[MyonKeminta]

Ok let's ignore the "<" case and only allow exactly equal. I'll change the name back to expected_for_update_ts.

[cfzjywxk]

LGTM