Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

session: reset CTEStorageMap before txn conflict and retry() #49399

Merged
merged 1 commit into from
Dec 13, 2023
Merged

session: reset CTEStorageMap before txn conflict and retry() #49399

merged 1 commit into from
Dec 13, 2023

Conversation

tiancaiamao
Copy link
Contributor

What problem does this PR solve?

Issue Number: close #46522

Problem Summary:

sctx.CTEStorageMap was set to nil in finishStmt, the code may call retry() in doCommitWithRetry() and rebuild the statement, during the rebuild process, sctx.CTEStorageMap type assertion fail.

What changed and how does it work?

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test
    • I checked and no code files have been changed.

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-triage-completed release-note-none Denotes a PR that doesn't merit a release note. labels Dec 12, 2023
@ti-chi-bot ti-chi-bot bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 12, 2023
@tiprow tiprow
Copy link

tiprow bot commented Dec 12, 2023

Hi @tiancaiamao. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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 kubernetes/test-infra repository.

@codecov Codecov
Copy link

codecov bot commented Dec 12, 2023 •
edited
Loading

Codecov Report

Merging #49399 (81321fc) into master (49f89a9) will increase coverage by 0.6476%.
Report is 27 commits behind head on master.
The diff coverage is 100.0000%.

Additional details and impacted files 
@@               Coverage Diff                @@
##             master     #49399        +/-   ##
================================================
+ Coverage   71.0490%   71.6967%   +0.6476%     
================================================
  Files          1368       1417        +49     
  Lines        401711     423152     +21441     
================================================
+ Hits         285412     303386     +17974     
- Misses        96443     100838      +4395     
+ Partials      19856      18928       -928
Flag Coverage Δ
integration 44.1406% <100.0000%> (?)
unit 71.0490% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 53.9663% <ø> (ø)
parser ∅ <ø> (∅)
br 47.6291% <ø> (-5.2644%) ⬇️

jackysp
jackysp previously approved these changes Dec 13, 2023
View reviewed changes
Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-chi-bot ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. needs-cherry-pick-release-5.4 Should cherry pick this PR to release-5.4 branch. needs-cherry-pick-release-6.1 Should cherry pick this PR to release-6.1 branch. needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. and removed do-not-merge/needs-triage-completed labels Dec 13, 2023
@ti-chi-bot ti-chi-bot bot removed the approved label Dec 13, 2023
jackysp
jackysp reviewed Dec 13, 2023
View reviewed changes
tests/integrationtest/t/executor/cte.test
@@ -153,3 +153,19 @@ create table t1(c1 int);
insert into t1 values(0), (1), (2), (3), (4), (5), (6), (7), (8), (9), (10), (11), (12), (13), (14), (15), (16), (17), (18), (19), (20), (21), (22), (23), (24), (25), (26), (27), (28), (29), (30), (31), (32), (33), (34), (35), (36), (37), (38), (39), (40), (41), (42), (43), (44), (45), (46), (47), (48), (49), (50), (51), (52), (53), (54), (55), (56), (57), (58), (59), (60), (61), (62), (63), (64), (65), (66), (67), (68), (69), (70), (71), (72), (73), (74), (75), (76), (77), (78), (79), (80), (81), (82), (83), (84), (85), (86), (87), (88), (89), (90), (91), (92), (93), (94), (95), (96), (97), (98), (99), (100), (101), (102), (103), (104), (105), (106), (107), (108), (109), (110), (111), (112), (113), (114), (115), (116), (117), (118), (119), (120), (121), (122), (123), (124), (125), (126), (127), (128), (129), (130), (131), (132), (133), (134), (135), (136), (137), (138), (139), (140), (141), (142), (143), (144), (145), (146), (147), (148), (149), (150), (151), (152), (153), (154), (155), (156), (157), (158), (159), (160), (161), (162), (163), (164), (165), (166), (167), (168), (169), (170), (171), (172), (173), (174), (175), (176), (177), (178), (179), (180), (181), (182), (183), (184), (185), (186), (187), (188), (189), (190), (191), (192), (193), (194), (195), (196), (197), (198), (199), (200), (201), (202), (203), (204), (205), (206), (207), (208), (209), (210), (211), (212), (213), (214), (215), (216), (217), (218), (219), (220), (221), (222), (223), (224), (225), (226), (227), (228), (229), (230), (231), (232), (233), (234), (235), (236), (237), (238), (239), (240), (241), (242), (243), (244), (245), (246), (247), (248), (249), (250), (251), (252), (253), (254), (255), (256), (257), (258), (259), (260), (261), (262), (263), (264), (265), (266), (267), (268), (269), (270), (271), (272), (273), (274), (275), (276), (277), (278), (279), (280), (281), (282), (283), (284), (285), (286), (287), (288), (289), (290), (291), (292), (293), (294), (295), (296), (297), (298), (299);
with recursive cte1(c1) as (select c1 from t1 union select c1 + 1 c1 from cte1 limit 1 offset 100) select * from cte1;
set tidb_max_chunk_size=default;

# TestIssue46522
Copy link
Member

@jackysp jackysp Dec 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot reproduce the original panic in the issue with this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

This is the error stack,

[err="type assertion for CTEStorageMap failed
github.com/pingcap/tidb/pkg/executor.(*executorBuilder).buildCTE
	/home/genius/project/src/github.com/pingcap/tidb/pkg/executor/builder.go:5277
github.com/pingcap/tidb/pkg/executor.(*executorBuilder).build
	/home/genius/project/src/github.com/pingcap/tidb/pkg/executor/builder.go:308
github.com/pingcap/tidb/pkg/executor.(*executorBuilder).buildInsert
	/home/genius/project/src/github.com/pingcap/tidb/pkg/executor/builder.go:976
github.com/pingcap/tidb/pkg/executor.(*executorBuilder).build
	/home/genius/project/src/github.com/pingcap/tidb/pkg/executor/builder.go:194
github.com/pingcap/tidb/pkg/executor.(*ExecStmt).buildExecutor
	/home/genius/project/src/github.com/pingcap/tidb/pkg/executor/adapter.go:1203
github.com/pingcap/tidb/pkg/executor.(*ExecStmt).Exec
	/home/genius/project/src/github.com/pingcap/tidb/pkg/executor/adapter.go:550
github.com/pingcap/tidb/pkg/session.(*session).retry
	/home/genius/project/src/github.com/pingcap/tidb/pkg/session/session.go:1177
github.com/pingcap/tidb/pkg/session.(*session).doCommitWithRetry
	/home/genius/project/src/github.com/pingcap/tidb/pkg/session/session.go:898
github.com/pingcap/tidb/pkg/session.(*session).CommitTxn
	/home/genius/project/src/github.com/pingcap/tidb/pkg/session/session.go:1001
github.com/pingcap/tidb/pkg/session.autoCommitAfterStmt
	/home/genius/project/src/github.com/pingcap/tidb/pkg/session/tidb.go:298
github.com/pingcap/tidb/pkg/session.finishStmt
	/home/genius/project/src/github.com/pingcap/tidb/pkg/session/tidb.go:260
github.com/pingcap/tidb/pkg/session.runStmt
	/home/genius/project/src/github.com/pingcap/tidb/pkg/session/session.go:2413
github.com/pingcap/tidb/pkg/session.(*session).ExecuteStmt
	/home/genius/project/src/github.com/pingcap/tidb/pkg/session/session.go:2236
github.com/pingcap/tidb/pkg/server.(*TiDBContext).ExecuteStmt
	/home/genius/project/src/github.com/pingcap/tidb/pkg/server/driver_tidb.go:293
github.com/pingcap/tidb/pkg/server.(*clientConn).handleStmt
	/home/genius/project/src/github.com/pingcap/tidb/pkg/server/conn.go:2045
github.com/pingcap/tidb/pkg/server.(*clientConn).handleQuery
	/home/genius/project/src/github.com/pingcap/tidb/pkg/server/conn.go:1835
github.com/pingcap/tidb/pkg/server.(*clientConn).dispatch
	/home/genius/project/src/github.com/pingcap/tidb/pkg/server/conn.go:1322
github.com/pingcap/tidb/pkg/server.(*clientConn).Run
	/home/genius/project/src/github.com/pingcap/tidb/pkg/server/conn.go:1101
github.com/pingcap/tidb/pkg/server.(*Server).onConn
	/home/genius/project/src/github.com/pingcap/tidb/pkg/server/server.go:701
runtime.goexit
	/home/genius/project/go/src/runtime/asm_amd64.s:1650
previous statement: insert into issue46522 with t1 as (select id+1 from  issue46522 where id = 1) select * from t1"]

I think they are the same type. @jackysp
doCommitWithRetry() -> retry() -> ... -> buildCTE() -> type assertion for CTEStorageMap failed

bb7133
bb7133 approved these changes Dec 13, 2023
View reviewed changes
Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-chi-bot ti-chi-bot
Copy link

ti-chi-bot bot commented Dec 13, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bb7133

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added approved lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Dec 13, 2023
@ti-chi-bot ti-chi-bot
Copy link

ti-chi-bot bot commented Dec 13, 2023

[LGTM Timeline notifier]

Timeline:

  • 2023-12-13 00:47:28.38368249 +0000 UTC m=+403539.420909403: ☑️ agreed by jackysp.
  • 2023-12-13 19:03:26.109007489 +0000 UTC m=+469297.146234415: ☑️ agreed by bb7133.

bb7133
bb7133 reviewed Dec 13, 2023
View reviewed changes
pkg/session/session.go
@@ -1146,6 +1146,7 @@ func (s *session) retry(ctx context.Context, maxCnt uint) (err error) {
for i, sr := range nh.history {
st := sr.st
s.sessionVars.StmtCtx = sr.stmtCtx
s.sessionVars.StmtCtx.CTEStorageMap = map[int]*executor.CTEStorages{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this missing code is a typical example of 'session context' issue of TiDB. /cc @lcwangchao

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I think the actual reason is the code path in retry is different with executing a new statement normally. In normal cases, ResetContextOfStmt is called when we enter a new statement, but in retry logic, we use ResetForRetry instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but we cannot totally reset the stmtctx for retry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The retry behavior is disabled by default.
It cause lost update #10075

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really hope one day we can get rid of the legacy code like this.
We can simplify the logic a lot!

@ti-chi-bot ti-chi-bot bot merged commit 1cfb3b9 into pingcap:master Dec 13, 2023
16 checks passed
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-5.4: #49442.

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Dec 13, 2023
@tiancaiamao @ti-chi-bot
This is an automated cherry-pick of pingcap#49399
d50e3ee 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot ti-chi-bot mentioned this pull request Dec 13, 2023
Open
13 tasks
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-6.1: #49443.

@ti-chi-bot ti-chi-bot mentioned this pull request Dec 13, 2023
Open
13 tasks
ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Dec 13, 2023
@tiancaiamao @ti-chi-bot
This is an automated cherry-pick of pingcap#49399
bf66051 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot ti-chi-bot mentioned this pull request Dec 13, 2023
Merged
13 tasks
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-6.5: #49444.

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Dec 13, 2023
@tiancaiamao @ti-chi-bot
This is an automated cherry-pick of pingcap#49399
ee62351 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-7.1: #49445.

@ti-chi-bot ti-chi-bot mentioned this pull request Dec 13, 2023
Merged
13 tasks
ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Dec 13, 2023
@tiancaiamao @ti-chi-bot
This is an automated cherry-pick of pingcap#49399
5246180 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-7.5: #49446.

@ti-chi-bot ti-chi-bot mentioned this pull request Dec 13, 2023
Merged
13 tasks
ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Dec 13, 2023
@tiancaiamao @ti-chi-bot
This is an automated cherry-pick of pingcap#49399
9bd3e2f 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@tiancaiamao tiancaiamao deleted the issue-45622 branch December 20, 2023 11:37
ti-chi-bot bot pushed a commit that referenced this pull request Dec 22, 2023
@ti-chi-bot
session: reset CTEStorageMap before txn conflict and retry() (#49399) (…
380a07a 
…#49444)

close #46522
ti-chi-bot bot pushed a commit that referenced this pull request Feb 6, 2024
@ti-chi-bot
session: reset CTEStorageMap before txn conflict and retry() (#49399) (…
c061545 
…#49446)

close #46522
ti-chi-bot bot pushed a commit that referenced this pull request Feb 26, 2024
@ti-chi-bot
session: reset CTEStorageMap before txn conflict and retry() (#49399) (…
2a0d877 
…#49445)

close #46522
tiancaiamao added a commit to tiancaiamao/tidb that referenced this pull request May 17, 2024
@tiancaiamao
session: reset CTEStorageMap before txn conflict and retry() pingcap#…
60f5ba1 
…49399
tiancaiamao added a commit to tiancaiamao/tidb that referenced this pull request May 17, 2024
@tiancaiamao
session: reset CTEStorageMap before txn conflict and retry() pingcap#…
7645e84 
…49399
ti-chi-bot bot pushed a commit that referenced this pull request May 17, 2024
@tiancaiamao
session: reset CTEStorageMap before txn conflict and retry() #49399 (#…
4f182e3 
…53353)

close #46522
ti-chi-bot bot pushed a commit that referenced this pull request May 17, 2024
@tiancaiamao
session: reset CTEStorageMap before txn conflict and retry() #49399 (#…
4b81451 
…53360)

close #46522
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackysp jackysp jackysp left review comments

@bb7133 bb7133 bb7133 approved these changes

@lcwangchao lcwangchao lcwangchao left review comments

@guo-shaoge guo-shaoge Awaiting requested review from guo-shaoge

Assignees
No one assigned
Labels
approved lgtm needs-cherry-pick-release-5.4 Should cherry pick this PR to release-5.4 branch. needs-cherry-pick-release-6.1 Should cherry pick this PR to release-6.1 branch. needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CTE query execute failed: type assertion for CTEStorageMap failed
5 participants
@tiancaiamao @ti-chi-bot @lcwangchao @bb7133 @jackysp