dml : dml read operation for cache table #29184
Conversation
|
[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
the
release-note-none
ti-chi-bot
added
do-not-merge/work-in-progress
| if err != nil { | ||
| return err | ||
| } | ||
| buffTxn, err := ctx.GetStore().BeginWithOption(tikv.DefaultStartTSOption().SetStartTS(0)) |
There was a problem hiding this comment.
The way to get a MemBuffer instance if tricky.
Add some comments here.
ti-chi-bot
added
size/XL
ti-chi-bot
removed
the
do-not-merge/work-in-progress
|
./rebuild |
|
@djshow832 @tisonkun PTAL thks |
executor/mem_reader.go
Outdated
| if cacheInfo.IsReadCacheTable { | ||
| tbl, ok := domain.GetDomain(ctx).InfoSchema().TableByID(cacheInfo.TableID) | ||
| if !ok { | ||
| return nil, errors.Trace(infoschema.ErrTableNotExists) |
There was a problem hiding this comment.
infoschema.ErrTableNotExists.GenStackByArgs() and provide the table name?
Grep the ErrTableNotExists to see how it's used.
There was a problem hiding this comment.
sorry. provide the table name and the db name is okay
executor/mem_reader.go
Outdated
| if !ok { | ||
| return nil, errors.Trace(infoschema.ErrTableNotExists) | ||
| } | ||
| cacheIter, err := tbl.(table.CachedTable).GetMemCache().Iter(rg.StartKey, rg.EndKey) |
There was a problem hiding this comment.
rg.StartKey, rg.EndKey is unnecessary?
The actual range should be [-int64, +int64) because the tbl.(table.CachedTable).GetMemCache() will not contain data from other tables.
sessionctx/stmtctx/stmtctx.go
Outdated
| @@ -88,7 +88,10 @@ type StatementContext struct { | |||
| MaybeOverOptimized4PlanCache bool | |||
| IgnoreExplainIDSuffix bool | |||
| IsStaleness bool | |||
|
|
|||
| CacheTableInfo struct { | |||
There was a problem hiding this comment.
What happen if "select * from t1 join t2 ..." where t1 and t2 are both cached table?
|
/cc @lcwangchao |
|
/run-check_dev_2 |
sessionctx/stmtctx/stmtctx.go
Outdated
| @@ -173,6 +172,8 @@ type StatementContext struct { | |||
| // Map to store all CTE storages of current SQL. | |||
| // Will clean up at the end of the execution. | |||
| CTEStorageMap interface{} | |||
| // cachedTables is used to store cache table id when it satisfies the cache read condition | |||
| cachedTables map[int64]bool | |||
There was a problem hiding this comment.
Maybe a slice is better here?
cachedTables []struct{
id int64
used bool
}
There won't be much cached table here, so the performance of slice should be good enough.
And this is in the performance critical path, reuse slice to avoid allocation is much easier, just cachedTables = cachedTables[:0]
|
@jayl-zxl: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
table/tables/cache.go
Outdated
| } | ||
|
|
||
| func (c *cachedTable) loadDataFromOriginalTable(ctx sessionctx.Context) error { | ||
| var mu sync.RWMutex |
There was a problem hiding this comment.
You should move this variable ti cachedTable
There was a problem hiding this comment.
Another question is that you should not lock entire function. Just locking setter for memebuffer is enough.
ti-chi-bot
added
status/LGT2
|
/merge |
|
This pull request has been accepted and is ready to merge. Commit hash: 6779d40
|
ti-chi-bot
added
the
status/can-merge
|
Close #29546 |
What problem does this PR solve?
Issue Number:
Problem Summary:
This is a subtask for #25293
What is changed and how it works?
This is a cache table with no read lock protocol to determine the version
So the read condition function that should be judged by ts. It returns true directly here.
About the judgment of the read lock, I will add it after the completion of this pr #29151
The time to read data from the original table is critical
When a cache table is read for the first time, we will return directly to failure. The backend go coroutine will update the lock information (of course, we did not implement this pr) and then read the data from the original table. So the second read will read the data from the cache
Like the temporary table, we use membuffer to store the data of the cache table
Check List
Tests
Release note