Better handling of partial dml query exec in transaction #9269

harshit-gangal · 2021-11-19T21:27:16Z

Description

This PR reverses the effect of partial dml query execution instead of rolling back the complete transaction when the transaction is explicitly created by the user.

Related Issue(s)

Closes RFC: Rollback of partial failed query in an explicit transaction #9266

Checklist

Tests were added or are not required
Documentation was added or is not required

Deployment Notes

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

…erse the affect of partial dml exec Signed-off-by: Harshit Gangal <harshit@planetscale.com>

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

…rmal) with correct commit order Signed-off-by: Harshit Gangal <harshit@planetscale.com>

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

…ack to a savepoint Signed-off-by: Harshit Gangal <harshit@planetscale.com>

…erriding it in nested execute calls Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>

…tant accross different vcursors Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

…ns when there is already an open transaction Signed-off-by: Harshit Gangal <harshit@planetscale.com>

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

go/test/endtoend/vtgate/partialfailure/main_test.go

go/vt/vtexplain/vtexplain.go

go/vt/vtgate/executor.go

go/vt/vtgate/executor_dml_test.go

go/vt/vtgate/executor_framework_test.go

systay · 2021-11-29T06:40:28Z

go/vt/vtgate/safe_session.go

@@ -40,6 +40,11 @@ type SafeSession struct {
 	mustRollback    bool
 	autocommitState autocommitState
 	commitOrder     vtgatepb.CommitOrder
+	savepointState  savepointState
+	// rollbackOnPartialExec is set if any DML was successfully


set to what? why is this a string?

rollbackOnPartialExec will store the rollback to <savepoint> sql query which is generated randomly when doing MarkSavepoint.

we should add ☝️ to the comment

systay · 2021-11-29T06:43:27Z

go/vt/vtgate/vcursor_impl.go

@@ -423,6 +421,8 @@ func (vc *vcursorImpl) StreamExecutePrimitive(primitive engine.Primitive, bindVa
 	return vterrors.New(vtrpcpb.Code_UNAVAILABLE, "upstream shards are not available")
 }

+const txRollback = "Rollback Transaction"


nit: weird placement for a const

earlier it was near to usage, then the code the moved,
now moved it back to the usage.

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

…ecord partial rollback query if the query even succeeds in one shard Signed-off-by: Harshit Gangal <harshit@planetscale.com>

go/vt/vtgate/executor_framework_test.go

go/vt/vtgate/plan_execute.go

go/vt/vtgate/safe_session.go

go/vt/vttablet/sandboxconn/sandboxconn.go

…ries Signed-off-by: Harshit Gangal <harshit@planetscale.com>

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

go/vt/vtexplain/vtexplain.go

go/vt/vtgate/autocommit_test.go

frouioui

Looks good to me. Agreed with @systay on #9269 (comment), we should avoid the repetition in the test suite to make adding/changing tests easier

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

systay · 2021-11-29T18:28:41Z

go/vt/vtexplain/vtexplain.go

+}
+
+func specialHandlingOfSavepoints(q *MysqlQuery) error {
+	if !strings.Contains(q.SQL, "savepoint") {


nit: can't we just skip this string check since we are looking at the type of the AST below?

This is just to avoid parsing overhead.

in vtexplain? doesn't feel like it's part of the code where we need to squeeze extra drops of performance.
¯_(ツ)_/¯

systay · 2021-11-29T18:33:12Z

go/vt/vtgate/safe_session.go

+// Default state is savepointStateNotSet,
+// if savepoint needed (spNeed true) then it will be set to savepointNeeded otherwise savepointNotNeeded.
+
+func (session *SafeSession) SetSavepointState(spNeed bool) {


comment missing

there was extra line, comments are there :)

systay · 2021-11-29T18:33:25Z

go/vt/vtgate/safe_session.go

+	return len(session.ShardSessions) > 0 || len(session.PreSessions) > 0 || len(session.PostSessions) > 0
+}
+
+func (session *SafeSession) GetSessions() []*vtgatepb.Session_ShardSession {


comment missing

added in #9294

systay

I was too late to accept it, but I would have if given the chance :)

LGTM 👍

harshit-gangal added Type: Enhancement Logical improvement (somewhere between a bug and feature) Breaking change Component: Query Serving release notes labels Nov 19, 2021

harshit-gangal mentioned this pull request Nov 19, 2021

Add a failing test case showing that a unique key error can cause Vitess to lose transaction state. #8984

Closed

harshit-gangal added 6 commits November 23, 2021 11:42

test: added parital query exec test in transaction for all modes

ab55480

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

feat: changed tx rollback on partial dml exec to use savepoint to rev…

2741788

…erse the affect of partial dml exec Signed-off-by: Harshit Gangal <harshit@planetscale.com>

test: correct the expected error message

cc6b6c5

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

feat: savepoint query to be executed on all session (pre, post and no…

d016c43

…rmal) with correct commit order Signed-off-by: Harshit Gangal <harshit@planetscale.com>

test: fix test to set the mode correctly

9ffbc4e

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

feat: support rollback on error in streaming (partial dml exec)

204cb1c

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

harshit-gangal force-pushed the partial-exec branch from b639caf to 204cb1c Compare November 23, 2021 06:12

harshit-gangal and others added 4 commits November 24, 2021 11:31

feat: in autocommit (implicit transaction) rollback the tx than rollb…

0bbf1db

…ack to a savepoint Signed-off-by: Harshit Gangal <harshit@planetscale.com>

feat: created a separate savepoint state in safeSession to prevent ov…

9cf8c86

…erriding it in nested execute calls Signed-off-by: Manan Gupta <manan@planetscale.com>

rename some structs and functions for better readability

67345d7

Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: moved rollbackOnPartialExec to safeSession so that it is persis…

21eb1b7

…tant accross different vcursors Signed-off-by: Manan Gupta <manan@planetscale.com>

GuptaManan100 force-pushed the partial-exec branch from dd18d1d to 21eb1b7 Compare November 24, 2021 10:55

harshit-gangal added 7 commits November 25, 2021 00:15

refactor: refactor the rollback logic to use same code method

dc799af

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

test: vtexplain to handle internal savepoints

5365f4f

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

fix: direct savepoint execution should only happen on shard connectio…

8a29a5b

…ns when there is already an open transaction Signed-off-by: Harshit Gangal <harshit@planetscale.com>

fix: max payload size requirement should ignore transactional statements

30b65a0

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

test: fix test function call to ignore savepoint

79b152c

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

test: added additional partial exec test cases

22af08d

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

test: vindex failure query message expectation changed

e364882

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

harshit-gangal marked this pull request as ready for review November 25, 2021 20:10

harshit-gangal requested review from frouioui and systay as code owners November 25, 2021 20:10

systay reviewed Nov 29, 2021

View reviewed changes

go/test/endtoend/vtgate/partialfailure/main_test.go Outdated Show resolved Hide resolved

systay reviewed Nov 29, 2021

View reviewed changes

go/vt/vtexplain/vtexplain.go Outdated Show resolved Hide resolved

systay reviewed Nov 29, 2021

View reviewed changes

go/vt/vtgate/executor.go Show resolved Hide resolved

systay reviewed Nov 29, 2021

View reviewed changes

go/vt/vtgate/executor.go Outdated Show resolved Hide resolved

systay reviewed Nov 29, 2021

View reviewed changes

go/vt/vtgate/executor_dml_test.go Outdated Show resolved Hide resolved

systay reviewed Nov 29, 2021

View reviewed changes

go/vt/vtgate/executor_framework_test.go Outdated Show resolved Hide resolved

systay reviewed Nov 29, 2021

View reviewed changes

harshit-gangal added 2 commits November 29, 2021 15:50

test: added failing unit test

68b1c48

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

feat: rollback to savepoint only once and reset the rollback query, r…

ecab6a3

…ecord partial rollback query if the query even succeeds in one shard Signed-off-by: Harshit Gangal <harshit@planetscale.com>

frouioui reviewed Nov 29, 2021

View reviewed changes

go/vt/vtgate/executor_framework_test.go Outdated Show resolved Hide resolved

go/vt/vtgate/plan_execute.go Outdated Show resolved Hide resolved

go/vt/vtgate/safe_session.go Outdated Show resolved Hide resolved

go/vt/vttablet/sandboxconn/sandboxconn.go Outdated Show resolved Hide resolved

harshit-gangal added 3 commits November 29, 2021 18:50

test: fix test query log framework to validate internal savepoint que…

21a4c5c

…ries Signed-off-by: Harshit Gangal <harshit@planetscale.com>

test: updated the test expectation

503f464

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

refactor: address review comments

07c7f54

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

harshit-gangal requested review from ajm188, doeg, GuptaManan100 and rohit-nayak-ps as code owners November 29, 2021 14:32

frouioui reviewed Nov 29, 2021

View reviewed changes

go/vt/vtexplain/vtexplain.go Outdated Show resolved Hide resolved

go/vt/vtgate/autocommit_test.go Outdated Show resolved Hide resolved

go/vt/vtgate/autocommit_test.go Outdated Show resolved Hide resolved

frouioui approved these changes Nov 29, 2021

View reviewed changes

harshit-gangal added 2 commits November 29, 2021 20:50

test: refactored tests

feb1335

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

test: refactored tests based on review comments

a794eb1

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

harshit-gangal merged commit 9c86727 into vitessio:main Nov 29, 2021

harshit-gangal deleted the partial-exec branch November 29, 2021 18:12

systay reviewed Nov 29, 2021

View reviewed changes

harshit-gangal mentioned this pull request May 7, 2022

partial dml execution logic ehancement #10223

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better handling of partial dml query exec in transaction #9269

Better handling of partial dml query exec in transaction #9269

harshit-gangal commented Nov 19, 2021 •

edited by deepthi

Loading

systay Nov 29, 2021

harshit-gangal Nov 29, 2021

systay Nov 29, 2021

systay Nov 29, 2021

harshit-gangal Nov 29, 2021

frouioui left a comment

systay Nov 29, 2021

harshit-gangal Nov 29, 2021

systay Nov 29, 2021 •

edited

Loading

systay Nov 29, 2021

harshit-gangal Nov 29, 2021

systay Nov 29, 2021

harshit-gangal Nov 29, 2021

systay left a comment

Better handling of partial dml query exec in transaction #9269

Better handling of partial dml query exec in transaction #9269

Conversation

harshit-gangal commented Nov 19, 2021 • edited by deepthi Loading

Description

Related Issue(s)

Checklist

Deployment Notes

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

frouioui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

systay Nov 29, 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

systay left a comment

Choose a reason for hiding this comment

harshit-gangal commented Nov 19, 2021 •

edited by deepthi

Loading

systay Nov 29, 2021 •

edited

Loading