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

*: Show key content in DuplicateEntry error message #828

Merged
merged 3 commits into from
Jan 5, 2016
Merged

*: Show key content in DuplicateEntry error message #828

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aca48ef
*: Show key content in DuplicateEntry error message
shenli Jan 5, 2016
a0f1c4b
kv: Address comments
shenli Jan 5, 2016
d8a6a93
tables: Address comment and refactor AddRecord methord
shenli Jan 5, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions kv/kv.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ const (
// transaction's commit.
// This option is an optimization for frequent checks during a transaction, e.g. batch inserts.
PresumeKeyNotExists Option = iota + 1
// PresumeKeyNotExistsError is the option key for error.
// When PresumeKeyNotExists is set and condition is not match, should throw the error.
PresumeKeyNotExistsError
)

// Retriever is the interface wraps the basic Get and Seek methods.
Expand Down
65 changes: 34 additions & 31 deletions kv/union_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,20 @@ var (
})
)

// conditionPair is used to store lazy check condition.
// If condition not match (value is not equal as expected one), returns err.
type conditionPair struct {
key Key
value []byte
err error
}

// UnionStore is an in-memory Store which contains a buffer for write and a
// snapshot for read.
type unionStore struct {
*BufferStore
snapshot Snapshot // for read
lazyConditionPairs MemBuffer // for delay check
snapshot Snapshot // for read
lazyConditionPairs map[string](*conditionPair) // for delay check
opts options
}

Expand All @@ -64,7 +72,7 @@ func NewUnionStore(snapshot Snapshot) UnionStore {
return &unionStore{
BufferStore: NewBufferStore(snapshot),
snapshot: snapshot,
lazyConditionPairs: &lazyMemBuffer{},
lazyConditionPairs: make(map[string](*conditionPair)),
opts: make(map[Option]interface{}),
}
}
Expand Down Expand Up @@ -121,7 +129,12 @@ func (us *unionStore) Get(k Key) ([]byte, error) {
v, err := us.MemBuffer.Get(k)
if IsErrNotFound(err) {
if _, ok := us.opts.Get(PresumeKeyNotExists); ok {
err = us.markLazyConditionPair(k, nil)
e, ok := us.opts.Get(PresumeKeyNotExistsError)
if ok {
err = us.markLazyConditionPair(k, nil, e.(error))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check the type conversion here in order to make sure it will not panic?

Copy link
Member Author

Choose a reason for hiding this comment

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

We should only put error into the entry with PresumeKeyNotExistsError as key.
But may be we should check if e is nil.

} else {
err = us.markLazyConditionPair(k, nil, ErrKeyExists)
}
if err != nil {
return nil, errors.Trace(err)
}
Expand All @@ -141,45 +154,36 @@ func (us *unionStore) Get(k Key) ([]byte, error) {
}

// markLazyConditionPair marks a kv pair for later check.
func (us *unionStore) markLazyConditionPair(k Key, v []byte) error {
if len(v) == 0 {
return errors.Trace(us.lazyConditionPairs.Delete(k))
func (us *unionStore) markLazyConditionPair(k Key, v []byte, e error) error {
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove the return error?

us.lazyConditionPairs[string(k)] = &conditionPair{
key: k,
Copy link
Contributor

Choose a reason for hiding this comment

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

The Key should be cloned before store in map, the caller may change the content after calling Get().
Or maybe we can use the map key directly in later check, then no need to store it here.

value: v,
err: e,
}
return errors.Trace(us.lazyConditionPairs.Set(k, v))
return nil
}

// CheckLazyConditionPairs implements the UnionStore interface.
func (us *unionStore) CheckLazyConditionPairs() error {
var keys []Key
it, err := us.lazyConditionPairs.Seek(nil)
if err != nil {
return errors.Trace(err)
}
for ; it.Valid(); it.Next() {
keys = append(keys, it.Key().Clone())
}
it.Close()

if len(keys) == 0 {
if len(us.lazyConditionPairs) == 0 {
return nil
}
values, err := us.snapshot.BatchGet(keys)
if err != nil {
return errors.Trace(err)
var keys []Key
Copy link
Contributor

Choose a reason for hiding this comment

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

This slice can be made with capacity of the map's length.

for _, v := range us.lazyConditionPairs {
keys = append(keys, v.key.Clone())
}
it, err = us.lazyConditionPairs.Seek(nil)
values, err := us.snapshot.BatchGet(keys)
if err != nil {
return errors.Trace(err)
}
defer it.Close()
for ; it.Valid(); it.Next() {
keyStr := string(it.Key())
if len(it.Value()) == 0 {
if _, exist := values[keyStr]; exist {
return errors.Trace(ErrKeyExists)

for k, v := range us.lazyConditionPairs {
if len(v.value) == 0 {
if _, exist := values[k]; exist {
return errors.Trace(v.err)
}
} else {
if bytes.Compare(values[keyStr], it.Value()) != 0 {
if bytes.Compare(values[k], v.value) != 0 {
return errors.Trace(ErrLazyConditionPairsNotMatch)
}
}
Expand All @@ -201,7 +205,6 @@ func (us *unionStore) DelOption(opt Option) {
func (us *unionStore) Release() {
us.snapshot.Release()
us.BufferStore.Release()
us.lazyConditionPairs.Release()
}

type options map[Option]interface{}
Expand Down
3 changes: 2 additions & 1 deletion kv/union_store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,8 @@ func (s *testUnionStoreSuite) TestLazyConditionCheck(c *C) {
c.Assert(err, IsNil)
c.Assert(v, BytesEquals, []byte("1"))

s.us.SetOption(PresumeKeyNotExists, 1)
s.us.SetOption(PresumeKeyNotExists, nil)
s.us.SetOption(PresumeKeyNotExistsError, ErrNotExist)
_, err = s.us.Get([]byte("2"))
c.Assert(terror.ErrorEqual(err, ErrNotExist), IsTrue)

Expand Down
19 changes: 16 additions & 3 deletions session_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/pingcap/tidb/sessionctx"
"github.com/pingcap/tidb/sessionctx/autocommit"
"github.com/pingcap/tidb/sessionctx/variable"
"github.com/pingcap/tidb/terror"
)

var _ = Suite(&testSessionSuite{})
Expand Down Expand Up @@ -1059,9 +1060,21 @@ func (s *testSessionSuite) TestIssue461(c *C) {
mustExecSQL(c, se1, "insert into test(id, val) values(1, 1);")
se2 := newSession(c, store, s.dbName)
mustExecSQL(c, se2, "begin;")
mustExecSQL(c, se2, "insert into test(id, val) values(1, 1);")
mustExecSQL(c, se2, "commit;")
mustExecFailed(c, se1, "commit;")
mustExecSQL(c, se2, "insert into test(id, val) values(2, 2);")
se3 := newSession(c, store, s.dbName)
mustExecSQL(c, se3, "begin;")
mustExecSQL(c, se3, "insert into test(id, val) values(1, 2);")
mustExecSQL(c, se3, "commit;")
_, err := se1.Execute("commit")
c.Assert(err, NotNil)
// Check error type and error message
c.Assert(terror.ErrorEqual(err, kv.ErrKeyExists), IsTrue)
c.Assert(err.Error(), Equals, "[kv:3]Duplicate entry '1' for key 'PRIMARY'")

_, err = se2.Execute("commit")
c.Assert(err, NotNil)
c.Assert(terror.ErrorEqual(err, kv.ErrKeyExists), IsTrue)
c.Assert(err.Error(), Equals, "[kv:3]Duplicate entry '2' for key 'val'")

se := newSession(c, store, s.dbName)
mustExecSQL(c, se, "drop table test;")
Expand Down
24 changes: 23 additions & 1 deletion table/tables/tables.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,12 +416,15 @@ func (t *Table) AddRecord(ctx context.Context, r []interface{}) (recordID int64,
if t.meta.PKIsHandle {
// Check key exists.
recordKey := t.RecordKey(recordID, nil)
e := kv.ErrKeyExists.Gen("Duplicate entry '%d' for key 'PRIMARY'", recordID)
txn.SetOption(kv.PresumeKeyNotExistsError, e)
_, err = txn.Get(recordKey)
if err == nil {
return recordID, kv.ErrKeyExists
return recordID, errors.Trace(e)
} else if !terror.ErrorEqual(err, kv.ErrNotExist) {
return 0, errors.Trace(err)
}
txn.DelOption(kv.PresumeKeyNotExistsError)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is better to use a defer to delete option, or the options value will be left undeleted if error occurs.

}

for _, v := range t.indices {
Expand All @@ -430,6 +433,24 @@ func (t *Table) AddRecord(ctx context.Context, r []interface{}) (recordID int64,
continue
}
colVals, _ := v.FetchValues(r)

if v.Unique || v.Primary {
// Pass pre-composed error to txn.
strVals := make([]string, 0, len(colVals))
for _, cv := range colVals {
cvs := "NULL"
if cv != nil {
cvs, err = types.ToString(cv)
if err != nil {
return 0, errors.Trace(err)
}
}
strVals = append(strVals, cvs)
}
entryKey := strings.Join(strVals, "-")
e := kv.ErrKeyExists.Gen("Duplicate entry '%s' for key '%s'", entryKey, v.Name)
txn.SetOption(kv.PresumeKeyNotExistsError, e)
}
if err = v.X.Create(bs, colVals, recordID); err != nil {
if terror.ErrorEqual(err, kv.ErrKeyExists) {
// Get the duplicate row handle
Expand All @@ -446,6 +467,7 @@ func (t *Table) AddRecord(ctx context.Context, r []interface{}) (recordID int64,
}
return 0, errors.Trace(err)
}
txn.DelOption(kv.PresumeKeyNotExistsError)
}

if err = t.LockRow(ctx, recordID); err != nil {
Expand Down