Fix cursor automatically moving to next position when delete and put are in the same TXN

[ahrtr]

Fix boltdb/bolt#357

cc @thatguystone @flowchartsman @benbjohnson

It's a bit funny that a bug isn't fixed until 8 years later.

Just copied the comment to explain the root cause,

	// If the deleted item is the first item in the node, we should keep
	// the cursor's position (decrease by 1). Otherwise, the cursor will
	// automatically move to the next item, and when clients call `Next`
	// method afterward, it will skip one item in the node.
	//
	// Note if there isn't any write on current page in current transaction,
	// then the cursor only iterates items against the page instead of the
	// in-memory node. Since the page is immutable, so we don't need to change
	// pos.index in such case.

[ahrtr]

cc @ptabor @fuweid @tjungblu as well

[fuweid]

The change looks good.

[ahrtr]

Actually the fix isn't enough. See the following two scenarios

Assuming we already inserted the following key/value pairs in the same TXN.

		for i := 0; i < count; i++ {
			k := make([]byte, 8)
			binary.BigEndian.PutUint64(k, uint64(i))
			b.Put(k, make([]byte, 100))
		}

• If clients starts deleting from an intermediate item instead of the first item all the way to the end. The number of remaining key/value pair is still incorrect.

The example below starts deleting from the third item (with index 2), so the number of remaining key/value pair should be 2, but actually it isn't. The reason is it skips one item each time after deleting one item.

		c := b.Cursor()
		c.First()
		c.Next()
		key, _ := c.Next()
		for key != nil {
			if err := c.Delete(); err != nil {
				return err
			}
			key, _ = c.Next()
		}

• Assuming you want to remove consecutive N key/value pair starting from a certain key. It will not remove the expected key/value pairs.

The example below removes consecutive 3 key/value pair starting from key 2. We expect it to remove key 2, 3, and 4, but actually it removes 2, 4 and 6. It's the same reason: it skips one item each time after deleting one item.

		c := b.Cursor()

		target := make([]byte, 8)
		binary.BigEndian.PutUint64(target, uint64(2))
		cnt := 3

		found := false
		for key, _ := c.First(); key != nil; key, _ = c.Next() {
			if bytes.Equal(key, target) {
				found = true
			}
			if found {
				if cnt > 0 {
					if err := c.Delete(); err != nil {
						return err
					}
					cnt--
				} else {
					break
				}
			}
		}

Root cause and solution

Just as I mentioned above, the root cause is that the cursor is based on a mutable in-memory node. Each time when removing an item, the cursor automatically moves to next item.

The solution should be maintain a immutable node copy for stable cursor. But it may have some impact on the performance.

[fuweid]

Good catch! I did pull your changes in my local and change the initPos with (c.First(), c.Next()) to test. But I didn't reproduce the issue 🥶

[ahrtr]

There is no cheap fix. In order to maintain an immutable node for stable cursor iteration, we need to copy the inodes for each node in the cursor iteration stack. Specifically we need to add field inodeSnapshot in elemRef, and copy node.inodes into inodeSnapshot when constructing each elemRef object during iteration.

// elemRef represents a reference to an element on a given page/node.
type elemRef struct {
	page          *common.Page
	node          *node
	inodeSnapshot common.Inodes
	index         int
}

Usually users won't delete key/value pairs either during iteration. So decided to just update doc to clarify the behavior. #614

[missinglink]

Agh funny, I just opened #629, came to the same conclusions.
Seems it's not as simple as I would have hoped 😢

[ahrtr]

Agh funny, I just opened #629, came to the same conclusions. Seems it's not as simple as I would have hoped 😢

Right, it isn't that simple. It doesn't deserve too much effort to fix it for now, unless lots of users drive it. Also it may either complicate the user interface or decrease the performance if we follow #611 (comment).

• If we clone the node.inodes each time when constructing the elemRef, then it may cause reduce of performance and more usage of memory.
• If we provide an option to let users to decide whether to clone node.inodes each time when constructing the elemRef, it will complicate the user interface and also hard to understand.

Let's update the doc (see #614) for now.

[ahrtr]

No plan to continue to work on this for now.

Note: the doc has already been updated in #614