planner: fix update panic when update in prepare and execute #26759

AilinKid · 2021-07-30T10:17:08Z

Signed-off-by: ailinkid [email protected]

What problem does this PR solve?

Issue Number: close #25997

What is changed and how it works?

What's Changed:

Background:
prepare and execute may have the difference schema to build the plan and run the statement. When we are in buildExecute, once we found the real statement of Execute is update read, we will substitute information schema with the lasted one. Latter, we use the latested schema to rebuild the plan, and run it.

Actually, `IS` is read only for all the travel path around optimize and exec. The `IS` and `TxnCtn.IS`
should always be the same, otherwise, it will cause some check fails. For example. `checkUpdateList`
panics in the plan building phase.

...
optimize(is) -> OptimizePreparedPlan(is) -> ... `is` changed
runstmt(is)
...

The modification of `is` is in the deep call chain. when function-call stack frame go back to the outer, since
the `is` struct is value copy model, the outer `is` is still the old one before calling the runstmt.

while the session TxnCtx hold the new `IS`, we can substitute the outer `is` once we found the schema
version difference before calling the runstmt

This pr solved several bad point.
1: do not convert point-table-scan to point-get if there are non-public column exists.
2: change schema in the deep call chain is not necessary and it's risky, since the outer schema is still old.
3: Binary-Protocol Execute

since it has been fetched in the entrance of optimize.(we remove these schema change code in the deep call chain, otherwise, the new plan built with new schema will encounter check error in execute phase with old schema)

4: Text-Protocol Execute

we need to fetch the latest schema as soon find it's execute stmtType with forUpdateRead flag as possible. here we choose to do this at the same level of optimize & runstmt.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)

use the test script in the issue.

before this pr, it will panic
after this, it can stably run

Release note

planner: fix update panic when update in prepare and executenner: fix update panic when update in prepare and execute

Signed-off-by: ailinkid <[email protected]>

ti-chi-bot · 2021-07-30T10:17:10Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

xhebox
you06

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Signed-off-by: ailinkid <[email protected]>

xhebox

I think it is kinda hacky. Maybe we can workaround the panic in another low-impact way.

planner/core/logical_plan_builder.go

xhebox · 2021-07-30T11:50:59Z

planner/optimize.go

 				is = domain.GetDomain(sctx).InfoSchema()
+				sctx.GetSessionVars().TxnCtx.InfoSchema = is


GetInfoSchema() is not equavalent to sctx.GetSessionVars().TxnCtx.InfoSchema, e.g. snapshot and stale infoschema. And it is intended that the schema of session is not always the global latest, it should be session-latest except some special cases like forUpdate. In fact, I found 2pc relied on this behavior.

I found 2pc relied on this behavior. I don't find this? where is it?

I only find it works on the resolveAccessPaths

you are right. these two information can be different! may be for this sort of forUpdateRead case, they should be the same.

2pc relied on schema checker and amender. Changing infoschema between optimize and execute may bypass the checker and fail the correctness of amender(for most cases? I don't know why forUpdateRead will ask for the latest exactly, seems it will cause inconsistency frequently if it is not the latest in some parallel cases)

You reminds me that I've suggested to remove these lines, because I have moved the logic of choosing correct infoschema to the outside https://github.com/pingcap/tidb/pull/26759/files#r679868151: these lines should be stub, the correct infoschema has been chosen. But got refused for risks, because the transaction reviewer seems can not track the whole execution path, too.

I've looked more closely, server/conn will dispatch prepare and execute into the one in session, so yes, the correct infoschema will be passed down from session down to here. And chasing a even newer infoschema between two closely coupled optimize and execute is non-sense for me.

We need a whole new proposal to define the behavior of concurrent DDL, because sync the latest = use the global latest. So all sessions will share the same infoschema as long as possible. It is been a gray area for a long time. I believe a talk to the transaction and runtime team is must.

session/session.go

executor/prepared.go

executor/executor.go

Signed-off-by: ailinkid <[email protected]>

djshow832 · 2021-08-05T08:12:04Z

planner/core/find_best_task.go

@@ -745,7 +755,7 @@ func (ds *DataSource) findBestTask(prop *property.PhysicalProperty, planCounter
 				p: dual,
 			}, cntPlan, nil
 		}
-		canConvertPointGet := len(path.Ranges) > 0 && path.StoreType == kv.TiKV
+		canConvertPointGet := len(path.Ranges) > 0 && path.StoreType == kv.TiKV && ds.isPointGetConvertableSchema()


What if executing normal statements without prepare-execute model? It affects those statements now.

it's ok, it's is a normal limitation.

djshow832 · 2021-08-05T08:15:20Z

planner/core/preprocess.go

@@ -157,6 +170,7 @@ type preprocessor struct {

 	// values that may be returned
 	*PreprocessorReturn
+	*PreprocessExecuteISUpdate


Why not name it Preprocessor like other members?

Why is PreprocessExecuteISUpdate necessary? How about just call IsExecuteForUpdateRead in ensureInfoSchema?

import circle problem, pass function poninter here is trying to do this

djshow832 · 2021-08-05T08:34:06Z

planner/optimize.go

@@ -77,6 +77,24 @@ func IsReadOnly(node ast.Node, vars *variable.SessionVars) bool {
 	return ast.IsReadOnly(node)
 }

+// IsExecuteForUpdateRead is used to check whether the statement is `execute` and target statement has a forUpdateRead flag.
+// If so, we will return the latest information schema.
+func IsExecuteForUpdateRead(node ast.Node, sctx sessionctx.Context) infoschema.InfoSchema {


This name sounds like a function returning a bool.

Signed-off-by: ailinkid <[email protected]>

you06 · 2021-08-05T11:17:27Z

/lgtm

ti-chi-bot · 2021-08-05T11:18:18Z

@you06: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

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.

you06 · 2021-08-05T14:38:00Z

/approve

AilinKid · 2021-08-05T14:44:59Z

/merge

ti-chi-bot · 2021-08-05T14:45:04Z

This pull request has been accepted and is ready to merge.

Commit hash: da070d7

zhouqiang-cl · 2021-08-05T15:33:53Z

/run-unit-tests

fix update panic when update in prepare and execute

ccf66fd

Signed-off-by: ailinkid <[email protected]>

ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 30, 2021

AilinKid requested a review from you06 July 30, 2021 10:18

AilinKid assigned xhebox and unassigned xhebox Jul 30, 2021

AilinKid requested a review from xhebox July 30, 2021 10:18

Merge branch 'master' into fix-update-in-prepare-execute

e66e134

AilinKid requested a review from sticnarf July 30, 2021 10:21

fmt

0b94fdc

Signed-off-by: ailinkid <[email protected]>

github-actions bot added sig/execution SIG execution sig/planner SIG: Planner sig/sql-infra SIG: SQL Infra labels Jul 30, 2021

xhebox reviewed Jul 30, 2021

View reviewed changes

AilinKid added 2 commits August 2, 2021 10:37

Merge branch 'master' into fix-update-in-prepare-execute

47b7c9a

remove risky code

843fabe

Signed-off-by: ailinkid <[email protected]>

zimulala self-requested a review August 2, 2021 07:50

AilinKid added 12 commits August 2, 2021 16:20

Merge branch 'master' into fix-update-in-prepare-execute

0644de4

remote unnecessary code

9ba22b5

Signed-off-by: ailinkid <[email protected]>

Merge branch 'master' into fix-update-in-prepare-execute

91127a1

fmt

39bdf6f

Signed-off-by: ailinkid <[email protected]>

resolve point get exit condition

7f75aac

Signed-off-by: ailinkid <[email protected]>

Merge branch 'master' into fix-update-in-prepare-execute

d04b0dd

remove controversial load schema in Execute

dbe84bf

Signed-off-by: ailinkid <[email protected]>

Merge branch 'master' into fix-update-in-prepare-execute

525a740

fix test

b358909

Signed-off-by: ailinkid <[email protected]>

Merge branch 'master' into fix-update-in-prepare-execute

f5d7f0a

fix test

1d5e335

Signed-off-by: ailinkid <[email protected]>

fix test

13e0a6f

Signed-off-by: ailinkid <[email protected]>

Merge branch 'master' into fix-update-in-prepare-execute

52f774d

morgo self-requested a review August 5, 2021 03:07

Merge branch 'master' into fix-update-in-prepare-execute

963f737

djshow832 reviewed Aug 5, 2021

View reviewed changes

rename func

ac14ea0

Signed-off-by: ailinkid <[email protected]>

you06 approved these changes Aug 5, 2021

View reviewed changes

Merge branch 'master' into fix-update-in-prepare-execute

da070d7

AilinKid added sig/transaction SIG:Transaction status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 5, 2021

ti-chi-bot added status/LGT1 Indicates that a PR has LGTM 1. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Aug 5, 2021

you06 approved these changes Aug 5, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 5, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 5, 2021

ti-chi-bot merged commit cc1f990 into pingcap:master Aug 5, 2021

xhebox mentioned this pull request Aug 9, 2021

*: infoschema compatibility with prepare #25080

Closed

zyguan mentioned this pull request Sep 13, 2021

Incorrect select result after point update nothing #28011

Closed

lcwangchao mentioned this pull request Nov 1, 2021

Prepared statements can't see temporary tables when the SQL involves multiple tables #29282

Closed

This was referenced Dec 23, 2021

planner: not use the latest schema when plan-cache is off #30959

Merged

TiDB internally panics with concurrent DML and DDL #30940

Closed

lcwangchao mentioned this pull request Apr 27, 2022

table schema is different for execute and select for update in transaction #34289

Closed

ti-chi-bot mentioned this pull request Dec 19, 2022

planner: not use the latest schema when plan-cache is off (#30959) #40046

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner: fix update panic when update in prepare and execute #26759

planner: fix update panic when update in prepare and execute #26759

AilinKid commented Jul 30, 2021 •

edited

Loading

ti-chi-bot commented Jul 30, 2021 •

edited

Loading

xhebox left a comment

xhebox Jul 30, 2021 •

edited

Loading

AilinKid Aug 2, 2021

AilinKid Aug 2, 2021

AilinKid Aug 2, 2021

xhebox Aug 2, 2021 •

edited

Loading

xhebox Aug 2, 2021 •

edited

Loading

djshow832 Aug 5, 2021

AilinKid Aug 5, 2021 •

edited

Loading

djshow832 Aug 5, 2021

djshow832 Aug 5, 2021

AilinKid Aug 5, 2021

djshow832 Aug 5, 2021

AilinKid Aug 5, 2021

you06 commented Aug 5, 2021

ti-chi-bot commented Aug 5, 2021

you06 commented Aug 5, 2021

AilinKid commented Aug 5, 2021

ti-chi-bot commented Aug 5, 2021

zhouqiang-cl commented Aug 5, 2021

		is = domain.GetDomain(sctx).InfoSchema()
		sctx.GetSessionVars().TxnCtx.InfoSchema = is

planner: fix update panic when update in prepare and execute #26759

planner: fix update panic when update in prepare and execute #26759

Conversation

AilinKid commented Jul 30, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Jul 30, 2021 • edited Loading

xhebox left a comment

Choose a reason for hiding this comment

xhebox Jul 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xhebox Aug 2, 2021 • edited Loading

Choose a reason for hiding this comment

xhebox Aug 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AilinKid Aug 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

you06 commented Aug 5, 2021

ti-chi-bot commented Aug 5, 2021

you06 commented Aug 5, 2021

AilinKid commented Aug 5, 2021

ti-chi-bot commented Aug 5, 2021

zhouqiang-cl commented Aug 5, 2021

AilinKid commented Jul 30, 2021 •

edited

Loading

ti-chi-bot commented Jul 30, 2021 •

edited

Loading

xhebox Jul 30, 2021 •

edited

Loading

xhebox Aug 2, 2021 •

edited

Loading

xhebox Aug 2, 2021 •

edited

Loading

AilinKid Aug 5, 2021 •

edited

Loading