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

executor: fix panic and update error data when table has column in write only state (#8792) #8904

Merged
merged 7 commits into from
Jan 2, 2019
Merged

executor: fix panic and update error data when table has column in write only state (#8792) #8904

merged 7 commits into from
Jan 2, 2019

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented Jan 2, 2019 •
edited by c4pt0r
Loading

What problem does this PR solve?

cherry-pick #8792

What is changed and how it works?

Check List

Tests

This change is Reviewable

@crazycs520
Copy link
Contributor Author

/run-all-tests tikv=release-2.0 tidb-test=release-2.0 pd=release-2.0

@crazycs520
Copy link
Contributor Author

@jackysp @lysu @winkyao PTAL

@crazycs520
Copy link
Contributor Author

/run-all-tests tikv=release-2.0 tidb-test=release-2.0 pd=release-2.0

@crazycs520 crazycs520 requested review from tiancaiamao and jackysp and removed request for tiancaiamao January 2, 2019 10:54
winkyao
winkyao reviewed Jan 2, 2019
View reviewed changes
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

winkyao
winkyao reviewed Jan 2, 2019
View reviewed changes
Copy link
Contributor

@winkyao winkyao left a comment •
edited
Loading

Choose a reason for hiding this comment

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

.

lysu
lysu reviewed Jan 2, 2019
View reviewed changes
executor/write.go Outdated Show resolved Hide resolved
executor/write.go
}
if onDup {
sc.AddAffectedRows(1)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

is this cherry-pick by #8233 & #6656？

maybe better cherry their testcase to here too, look good to me but cannot confirm it works well without testcase... 🤣

Copy link
Contributor

Choose a reason for hiding this comment

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

@crazycs520 Please pick the test cases.

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 have cherry-picked test of #6656, But I can't cherry-pick the test of #8233, There have some other problems.

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 add a part of tests from #8233, PTAL. @lysu

@crazycs520
Copy link
Contributor Author

/run-all-tests tikv=release-2.0 tidb-test=release-2.0 pd=release-2.0

1 similar comment
@crazycs520
Copy link
Contributor Author

/run-all-tests tikv=release-2.0 tidb-test=release-2.0 pd=release-2.0

@crazycs520
Copy link
Contributor Author

/run-unit-test tikv=release-2.0 tidb-test=release-2.0 pd=release-2.0

tiancaiamao
tiancaiamao reviewed Jan 2, 2019
View reviewed changes
executor/write.go
}

tid := t.Meta().ID
ctx.StmtAddDirtyTableOP(DirtyTableDeleteRow, tid, h, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why dirty table operation is changed, it should be irrelevant

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 row add to dirty table have move to AddRecord and UpdateRecord and RemoveRecord.
Because AddRecord will backfill default value to row for write-only column. If dirty table doesn't have the value of write-only column, update will panic in transaction. See #8792 description Problem 1.

@crazycs520
Copy link
Contributor Author

/run-all-tests tikv=release-2.0 tidb-test=release-2.0 pd=release-2.0

1 similar comment
@crazycs520
Copy link
Contributor Author

/run-all-tests tikv=release-2.0 tidb-test=release-2.0 pd=release-2.0

@crazycs520
Copy link
Contributor Author

/run-all-tests tikv=release-2.0 tidb-test=release-2.0 pd=release-2.0

1 similar comment
@crazycs520
Copy link
Contributor Author

/run-all-tests tikv=release-2.0 tidb-test=release-2.0 pd=release-2.0

@crazycs520
Copy link
Contributor Author

/run-unit-test tikv=release-2.0 tidb-test=release-2.0 pd=release-2.0

@zyguan
Copy link
Contributor

zyguan commented Jan 2, 2019

/run-unit-test tikv=release-2.0 tidb-test=release-2.0 pd=release-2.0 gofail=etcd-io

@lysu
Copy link
Contributor

lysu commented Jan 2, 2019

LGTM

@lysu lysu added the status/LGT2 Indicates that a PR has LGTM 2. label Jan 2, 2019
@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Jan 2, 2019
@winkyao winkyao merged commit ef9b85c into pingcap:release-2.0 Jan 2, 2019
tiancaiamao added a commit that referenced this pull request Jan 2, 2019
@tiancaiamao
Revert "executor: fix panic and update error data when table has colu…
cc45398 
…mn in write only state (#8792) (#8904)"

This reverts commit ef9b85c.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@winkyao winkyao winkyao left review comments

@lysu lysu lysu left review comments

@tiancaiamao tiancaiamao tiancaiamao approved these changes

@jackysp jackysp Awaiting requested review from jackysp

Assignees
No one assigned
Labels
sig/execution SIG execution status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@crazycs520 @zyguan @lysu @tiancaiamao @winkyao