*: Introduce PessimisticRRTxnContextProvider for pessimistic repeatable read txn #35158
Conversation
Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
ti-chi-bot
added
release-note-none
|
Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/248e8260fe5fc223af153f98f9214479e7926d65 |
Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>
Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>
| } | ||
|
|
||
| mayOptimizeForPointGet := false | ||
| if v, ok := plan.(*plannercore.PhysicalLock); ok { |
There was a problem hiding this comment.
Some cases can also use this optimization, for example:
select v from t where id=1 and v>1 for update; -- Projection <- SelectLock <- Selection <- PointGet'
select * from t where id=1 union all select * from t where id=2; -- Union <- SelectLock <- PointGet
select * from t where id=1 union select * from t where id=1; -- HashAgg <- Union <- SelectLock <- PointGetI think BatchPointGet can also use this optimization, but the old code did not do it, any problem if we do it now?
@cfzjywxk
There was a problem hiding this comment.
Seems it's ok as the locking behaviors are similar. Actually different behaviors for batch/point get have introduced us much trouble for us just like #32045.
There was a problem hiding this comment.
BatchPointGet does not use txnManager.GetStmtForUpdateTS() in executorBuilder.getSnapshotTS(), which means it will not acquire ts from PD.
sessiontxn/interface.go
Outdated
| @@ -120,7 +122,7 @@ type TxnContextProvider interface { | |||
| // OnStmtRetry is the hook that should be called when a statement is retried internally. | |||
| OnStmtRetry(ctx context.Context) error | |||
| // Advise is used to give advice to provider | |||
| Advise(tp AdviceType) error | |||
| Advise(tp AdviceType, val []any) error | |||
There was a problem hiding this comment.
Try to avoid using any type but a concrete type in the interface definition, it would make it difficult to use and maintain.
There was a problem hiding this comment.
It's hard to construct a concrete type here as the parameters of different advice type differs a lot. Could you give some suggestions?
There was a problem hiding this comment.
It depends on the design of what's planned to be passed inside and how the parameters would be used. The most commonly used way is to abstract some interface or trait to describe the meaning of the input parameters, for example, you could abstract a new type named TxnAdvice which contains both advice type and value, or abstract a new interface which defines what should be passed in as the advice parameters.
The main thing is to answer what's an advice and how to describe it, what's the design?, but not just leave something like whatever type is acceptable, perhaps some will work some would not especially defining interfaces.
In general, don't use the any or inteface{} types it's not maintainable and other people have no idea what should be passed or how to build another advice type.
| // Because stale read will use `staleread.StalenessTxnContextProvider` for query, so if `staleread.IsStmtStaleness()` | ||
| // returns true in other providers, it means the current statement is `BeginStmt` with stale read | ||
| func (p *baseTxnContextProvider) isBeginStmtWithStaleRead() bool { | ||
| return staleread.IsStmtStaleness(p.sctx) |
There was a problem hiding this comment.
Where's the logic checking isBeiginStmt, the function name is a bit confusing.
| } | ||
|
|
||
| mayOptimizeForPointGet := false | ||
| if v, ok := plan.(*plannercore.PhysicalLock); ok { |
There was a problem hiding this comment.
Seems it's ok as the locking behaviors are similar. Actually different behaviors for batch/point get have introduced us much trouble for us just like #32045.
|
It seems to me some part of this change has overlaps/conflicts with #35131? Is there a dependency between the two? |
These two PRs will be unified eventually. The later one may need to solve the merge conflictss. |
|
/run-unit-test |
Signed-off-by: SpadeA-Tang <u6748471@anu.edu.au>
ti-chi-bot
added
status/LGT2
|
/merge |
|
This pull request has been accepted and is ready to merge. Commit hash: f2ee63f
|
ti-chi-bot
added
the
status/can-merge
TiDB MergeCI notify🔴 Bad News! [1] CI still failing after this pr merged.
|
Signed-off-by: SpadeA-Tang u6748471@anu.edu.au
What problem does this PR solve?
Issue Number: close #35129
What is changed and how it works?
todo:
Check List
Tests
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.