fix issue #463 #476

winkyao · 2015-10-28T14:26:23Z

Merge pull request from pingcap/tidb

to delete the discard index keys when we enconter error in table.AddRecord.(which fix issue 463: #463)

Merge pull request from pingcap/tidb

shenli · 2015-10-29T02:19:09Z

table/tables/tables.go

+			break
+		}
+		colVals, _ := v.FetchValues(r)
+		_ = v.X.Delete(txn, colVals, recordID)


This maybe not correct. You want to delete dirty data, but this may delete the data in kv.
For example there is a row [2, 2] already in kv store and you insert [2, 1] and meet ErrKeyExists. In this case, you may delete the correct index for row [2, 2].
I think it is better to check constraints before creating index.

I know this, but in here, it is correct, because if the index key is already exist, it will not be delete in the delDirtyIndexKey, so the delDirtyIndexKey will only delete the index key in dirty strore that created incorrectly. Please note:

if i >= createdIdx { break }

and in your mention case, it will not delete the index for row [2, 2]..., because the createdIdx is 0.

"I think it is better to check constraints before creating index." imagine if you have more than one unique index..

Yes you are right. It is ok here.
But when executing update statements, you will meet similar problem. There is also a bug in update statement, we update row data before updating index. So if meet keyexist error when updating index, unionstore is still dirty.
So I think it may be better to provide some mechanism to check index constraints before operating unionstore.

um, sorry...I am not familiar with the update statements, I will read it then find anathor better solution. :-)

@winkyao you can supply a CheckContraint function or other, so that before insert and update, we can use this to check whether we can handle this row or not. :-)

I have some troubles that I check the index are all available to create, then I create the indices, but the check and create is not a atomic operation, so I may fail when I create one of the indices which is created by other people in the interval..

maybe I use delDirtyIndexKey in update statement is a ugly, but a available solution?
@siddontang @shenli

It can be detected at the commit stage and will cause error. The whole txn will rollback.
I think it is another scenario.

It's complicated. For issue #463, we should rollback, since the state is auto_commit=1.

siddontang · 2015-10-29T02:35:28Z

I think we should check constraints before handing row data. :-)

table/tables/tables.go: before create index add checkCreateUnqIdxAvail and checkUpdateUnqIdxAvail tidb_test.go: add issue #463 test case for fix the issue #463: #463

winkyao · 2015-10-29T09:07:14Z

@siddontang @shenli

ngaut · 2015-10-29T09:35:38Z

Thanks @winkyao
Have you tried some tests like:

Update the same value multiple times.
Delete and insert again.
All of those operations inside a transaction.

winkyao · 2015-10-29T09:37:59Z

um...@ngaut some test run two parallel transaction.

siddontang · 2015-10-29T09:38:01Z

kv/index_iter.go

@@ -237,6 +271,27 @@ func (c *kvIndex) Seek(txn Transaction, indexedValues []interface{}) (iter Index
 	return &indexIter{it: it, idx: c, prefix: c.prefix}, hit, nil
 }

+// check if the index is duplicated, only check the unique index
+func (c *kvIndex) CheckUnique(txn Transaction, indexedValues []interface{}, h int64) (duplicate bool, err error) {


CheckDuplicated may be better with above comment.

Same comment problem.

…ze the comment.

winkyao · 2015-10-29T09:55:06Z

@siddontang Done!

ngaut · 2015-10-29T12:23:17Z

kv/index_iter.go

+// If the index is unique and there already exists an entry with the same key, Create will continue, and failed when commit
+// MustCreate must call after the CheckDuplicated, which tell there is no duplicate key, and if someone create the same index between CheckDuplicated
+// and MustCreate, finally our transaction commit will fail, so it is ok.
+func (c *kvIndex) MustCreate(txn Transaction, indexedValues []interface{}, h int64) error {


There is no reason to fail if we call it "Must". "Create" is fine.

But the Index.Create is already exist..so what name you think is better?

siddontang · 2015-11-01T23:41:01Z

@winkyao The newest master has already supported index Exist function, you can use it directly.

winkyao · 2015-11-03T09:16:28Z

@siddontang I have other works these days, so I can not fix it recently. May you fix it? I am very sorry.

siddontang · 2015-11-03T10:19:28Z

Thanks, if we will fix this issue, we will tell you :-)

disksing · 2015-12-04T05:21:33Z

Hi @winkyao :
We have fixed this issue in another approach. Please refer #670 for more details.
I will close this PR. Thanks for your contribution.

* support s3 seek end * support nil for WalkOption * try parse range info from respose * fix test * fix lint * fix a bug in s3 WalkDir * update * update * fix * loop read s3 reader * fix lint * fix more lint * fmt * use io.ReadFull instead of manual loop * fix * fix * add a comment for the reason to put in struct * add a comment

* retry write&ingest range * more robust retry ranges * fix * simplify code and add a log Co-authored-by: lance6716 <[email protected]> Co-authored-by: kennytm <[email protected]>

…shutting down (pingcap#476) * test for small ttl of etcd key Signed-off-by: zeminzhou <[email protected]> * domain, tidb-server: stop launching new auto analyze job when shutting down (pingcap#41346) close pingcap#41318 * clean other change Signed-off-by: zeminzhou <[email protected]> --------- Signed-off-by: zeminzhou <[email protected]> Co-authored-by: Yifan Xu <[email protected]>

winkyao added 3 commits October 28, 2015 17:27

Merge pull request #2 from pingcap/master

8772365

Merge pull request from pingcap/tidb

table/tables/tables.go: add table.delDirtyIndexKey func

8ac556d

to delete the discard index keys when we enconter error in table.AddRecord.(which fix issue 463: #463)

Merge pull request #3 from pingcap/master

83a71b4

Merge pull request from pingcap/tidb

shenli reviewed Oct 29, 2015
View reviewed changes

winkyao added 2 commits October 29, 2015 16:31

kv: add two Index interface func

1da983a

table/tables/tables.go: before create index add checkCreateUnqIdxAvail and checkUpdateUnqIdxAvail tidb_test.go: add issue #463 test case for fix the issue #463: #463

Merge branch 'master' of https://github.com/winkyao/tidb

71b9cd7

siddontang reviewed Oct 29, 2015
View reviewed changes

refactor func name from CheckUnique to CheckDuplicated, and standardi…

3428404

…ze the comment.

winkyao added 2 commits October 29, 2015 19:35

merge from ping/tidb

0eabdd4

Merge branch 'pingcap-master'

4ef2722

ngaut reviewed Oct 29, 2015
View reviewed changes

disksing closed this Dec 4, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix issue #463 #476

fix issue #463 #476

winkyao commented Oct 28, 2015

shenli Oct 29, 2015

winkyao Oct 29, 2015

shenli Oct 29, 2015

winkyao Oct 29, 2015

siddontang Oct 29, 2015

winkyao Oct 29, 2015

shenli Oct 29, 2015

ngaut Oct 29, 2015

siddontang commented Oct 29, 2015

winkyao commented Oct 29, 2015

ngaut commented Oct 29, 2015

winkyao commented Oct 29, 2015

siddontang Oct 29, 2015

siddontang Oct 29, 2015

winkyao commented Oct 29, 2015

ngaut Oct 29, 2015

winkyao Oct 29, 2015

siddontang commented Nov 1, 2015

winkyao commented Nov 3, 2015

siddontang commented Nov 3, 2015

disksing commented Dec 4, 2015

fix issue #463 #476

fix issue #463 #476

Conversation

winkyao commented Oct 28, 2015

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddontang commented Oct 29, 2015

winkyao commented Oct 29, 2015

ngaut commented Oct 29, 2015

winkyao commented Oct 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winkyao commented Oct 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddontang commented Nov 1, 2015

winkyao commented Nov 3, 2015

siddontang commented Nov 3, 2015

disksing commented Dec 4, 2015