From c9757f4d18c71bc63de470a1f9c24db76a8095a5 Mon Sep 17 00:00:00 2001 From: Bryan Matsuo Date: Sun, 22 Jan 2017 15:06:56 -0800 Subject: [PATCH 1/4] Add a test for Cursor.Get with op lmdb.Set to try and replicate #96 --- lmdb/cursor_test.go | 56 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-) diff --git a/lmdb/cursor_test.go b/lmdb/cursor_test.go index 0d387c14..f5f1067e 100644 --- a/lmdb/cursor_test.go +++ b/lmdb/cursor_test.go @@ -249,7 +249,7 @@ func TestCursor_Get_KV(t *testing.T) { t.Errorf("unexpected key: %q (not %q)", k, "key") } if string(v) != "1" { - t.Errorf("unexpected key: %q (not %q)", k, "1") + t.Errorf("unexpected value: %q (not %q)", k, "1") } _, _, err = cur.Get([]byte("key"), []byte("1"), GetBoth) @@ -260,6 +260,60 @@ func TestCursor_Get_KV(t *testing.T) { } } +func TestCursor_Get_op_Set(t *testing.T) { + env := setup(t) + defer clean(env, t) + + var dbi DBI + err := env.Update(func(txn *Txn) (err error) { + dbi, err = txn.OpenDBI("testdb", Create|DupSort) + return err + }) + if err != nil { + t.Errorf("%s", err) + return + } + + err = env.Update(func(txn *Txn) (err error) { + put := func(k, v []byte) { + if err == nil { + err = txn.Put(dbi, k, v, 0) + } + } + put([]byte("k1"), []byte("v11")) + put([]byte("k1"), []byte("v12")) + put([]byte("k1"), []byte("v13")) + put([]byte("k2"), []byte("v21")) + put([]byte("k2"), []byte("v22")) + put([]byte("k2"), []byte("v23")) + return err + }) + if err != nil { + t.Errorf("%s", err) + } + + err = env.View(func(txn *Txn) (err error) { + cur, err := txn.OpenCursor(dbi) + if err != nil { + return err + } + k, v, err := cur.Get([]byte("k2"), nil, Set) + if err != nil { + return err + } + if string(k) != "k2" { + t.Errorf("unexpected key: %q (not %q)", k, "k2") + } + if string(v) != "v21" { + t.Errorf("unexpected value: %q (not %q)", v, "v21") + } + return nil + }) + if err != nil { + t.Errorf("%s", err) + } +} + func TestCursor_Get_DupFixed(t *testing.T) { env := setup(t) defer clean(env, t) From 384db509b860652c849cd5859723c5d5a4a11d4d Mon Sep 17 00:00:00 2001 From: Bryan Matsuo Date: Fri, 27 Jan 2017 05:36:41 -0800 Subject: [PATCH 2/4] Add missing call to Cursor.Close() --- lmdb/cursor_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lmdb/cursor_test.go b/lmdb/cursor_test.go index f5f1067e..b59aa347 100644 --- a/lmdb/cursor_test.go +++ b/lmdb/cursor_test.go @@ -297,6 +297,8 @@ func TestCursor_Get_op_Set(t *testing.T) { if err != nil { return err } + defer cur.Close() + k, v, err := cur.Get([]byte("k2"), nil, Set) if err != nil { return err @@ -307,6 +309,7 @@ func TestCursor_Get_op_Set(t *testing.T) { if string(v) != "v21" { t.Errorf("unexpected value: %q (not %q)", v, "v21") } + return nil }) if err != nil { From 345b86e9ba8b69169122e7bfbda0c31fd85ae6ba Mon Sep 17 00:00:00 2001 From: Bryan Matsuo Date: Fri, 27 Jan 2017 23:53:31 -0800 Subject: [PATCH 3/4] Fix test to properly replicate #96 --- lmdb/cursor_test.go | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/lmdb/cursor_test.go b/lmdb/cursor_test.go index 5cd1d83e..3514c6d9 100644 --- a/lmdb/cursor_test.go +++ b/lmdb/cursor_test.go @@ -261,7 +261,7 @@ func TestCursor_Get_KV(t *testing.T) { } } -func TestCursor_Get_op_Set(t *testing.T) { +func TestCursor_Get_op_Set_bytesBuffer(t *testing.T) { env := setup(t) defer clean(env, t) @@ -300,15 +300,34 @@ func TestCursor_Get_op_Set(t *testing.T) { } defer cur.Close() - k, v, err := cur.Get([]byte("k2"), nil, Set) + // Create bytes.Buffer values containing a amount of bytes. Byte + // slices returned from buf.Bytes() have a history of tricking the cgo + // argument checker. + var kbuf bytes.Buffer + kbuf.WriteString("k2") + + k, _, err := cur.Get(kbuf.Bytes(), nil, Set) if err != nil { return err } - if string(k) != "k2" { - t.Errorf("unexpected key: %q (not %q)", k, "k2") + if string(k) != kbuf.String() { + t.Errorf("unexpected key: %q (not %q)", k, kbuf.String()) + } + + // No guarantee is made about the return value of mdb_cursor_get when + // MDB_SET is the op, so its value is not checked as part of this test. + // That said, it is important that Cursor.Get not panic if given a + // short buffer as an input value for a Set op (despite that not really + // having any significance) + var vbuf bytes.Buffer + vbuf.WriteString("v22") + + k, _, err = cur.Get(kbuf.Bytes(), vbuf.Bytes(), Set) + if err != nil { + return err } - if string(v) != "v21" { - t.Errorf("unexpected value: %q (not %q)", v, "v21") + if string(k) != kbuf.String() { + t.Errorf("unexpected key: %q (not %q)", k, kbuf.String()) } return nil From dca305732b038a87b52f7eb5d7fc643dde045df4 Mon Sep 17 00:00:00 2001 From: Bryan Matsuo Date: Sat, 28 Jan 2017 00:11:28 -0800 Subject: [PATCH 4/4] Cursor.Get handles key return values specicially for Set Fixes #96 Issue #56 illustrated that working with Go memory using "C-style" access patterns does not work well in modern Go runtimes. That problem was thought to be properly addressed, but #96 showed a lingering case where Set does not modify MDB_val key passed to mdb_cursor_get. This commit addresses this lingering case by special-casing return value processing for the Set operation of Cursor.Get. If LMDB will not modify the MDB_val, the returned key must use non-standard methods for copying the value. Likewise, when txn.RawRead is true the Cursor.Get must use a special method to ensure that the returned key shares memory with the corresponding input argument. --- lmdb/cursor.go | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/lmdb/cursor.go b/lmdb/cursor.go index b82caea1..b9c89dbb 100644 --- a/lmdb/cursor.go +++ b/lmdb/cursor.go @@ -135,6 +135,11 @@ func (c *Cursor) DBI() DBI { // returned by Get reference readonly sections of memory that must not be // accessed after the transaction has terminated. // +// In a Txn with RawRead set to true the Set op causes the returned key to +// share its memory with setkey (making it writable memory). In a Txn with +// RawRead set to false the Set op returns key values with memory distinct from +// setkey, as is always the case when using RawRead. +// // Get ignores setval if setkey is empty. // // See mdb_cursor_get. @@ -152,11 +157,30 @@ func (c *Cursor) Get(setkey, setval []byte, op uint) (key, val []byte, err error *c.txn.val = C.MDB_val{} return nil, nil, err } - k := c.txn.bytes(c.txn.key) - v := c.txn.bytes(c.txn.val) + + // When MDB_SET is passed to mdb_cursor_get its first argument will be + // returned unchanged. Unfortunately, the normal slice copy/extraction + // routines will be bad for the Go runtime when operating on Go memory + // (panic or potentially garbage memory reference). + if op == Set { + if c.txn.RawRead { + key = setkey + } else { + p := make([]byte, len(setkey)) + copy(p, setkey) + key = p + } + } else { + key = c.txn.bytes(c.txn.key) + } + val = c.txn.bytes(c.txn.val) + + // Clear transaction storage record storage area for future use and to + // prevent dangling references. *c.txn.key = C.MDB_val{} *c.txn.val = C.MDB_val{} - return k, v, nil + + return key, val, nil } // getVal0 retrieves items from the database without using given key or value