*: forbid stale read in local temporary tables#26976
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. DetailsReviewer can indicate their review by submitting an approval review. |
ti-chi-bot
added
release-note-none
|
@tisonkun: Request changes is only allowed for the reviewers in list. DetailsIn 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. |
ti-chi-bot
added
the
needs-rebase
ti-chi-bot
removed
the
needs-rebase
ti-chi-bot
added
size/M
|
/run-all-tests |
|
/run-check_dev_2 |
|
/run-check_dev_2 |
| tk.MustExec("drop table tmp6") | ||
| tk.MustExec("create table tmp6 (id int primary key);") | ||
| tk.MustQuery("select * from tmp6 as of timestamp(@a) where id=1;").Check(testkit.Rows("1")) | ||
| tk.MustQuery("select * from tmp4 as of timestamp(@a), tmp3 as of timestamp(@a) where tmp3.id=1;") |
| if p.err != nil { | ||
| return | ||
| } | ||
| p.InfoSchema = is.(infoschema.InfoSchema) |
There was a problem hiding this comment.
What will happen if is is not a infoschema.InfoSchema
p.InfoSchema is nil and no error set, then maybe the code panic in some other place?
| }() | ||
|
|
||
| if execOption.SnapshotTS != 0 { | ||
| se.sessionVars.SnapshotInfoschema, err = getSnapshotInfoSchema(s, execOption.SnapshotTS) |
There was a problem hiding this comment.
Why ExecRestrictedStmt need to consider the temporary table? it's usually used to execute some internal SQL statement, but no internal SQL will use the temporary table!
Can you ever construct a case that the infoschema obtained here visit the local temporary table?
| if p.LastSnapshotTS != 0 { | ||
| dom := domain.GetDomain(p.ctx) | ||
| p.InfoSchema, p.err = dom.GetSnapshotInfoSchema(p.LastSnapshotTS) | ||
| is, err := p.ctx.GetSnapshotInfoSchema(p.LastSnapshotTS) |
There was a problem hiding this comment.
Is this the only place that needs to get the snapshot infoschema with the temporary table? If so, I prefer manually use
&infoschema.TemporaryTableAttachedInfoSchema{
InfoSchema: is,
LocalTemporaryTables: local.(*infoschema.LocalTemporaryTables)
}
instead of expose a new method to the sessionctx.Context.
The bigger the interface, the weaker the abstraction – Rob Pike
|
/run-check_dev2 |
tiancaiamao
changed the title
sylzd
force-pushed
the
temp_table_25937
branch
from
1907d00 to
6ec93be
Compare
|
duplicated with #27270 |
6 participants
What problem does this PR solve?
Issue Number: close #25937
Problem Summary:
What is changed and how it works?
Proposal: xxx
What's Changed:
TestInvalidReadTemporaryTablepatched, which can reduce much replicated code.How it Works:
snapshot does not exit in local temporary, so
GetSnapshotInfoSchemacan not work. I added a check afterGetSnapshotInfoSchema, whether normal table exits or not.Check List
Tests
Side effects
Documentation
Release note