CreateBucketIfNotExists try opening bucket before creating

[cenkalti]

Fixes #118

Benchmark with 1 million keys (137 MB database):

BenchmarkCreateBucketIfNotExists-8      	     145	   8594488 ns/op	   18018 B/op	      34 allocs/op
BenchmarkCreateBucketIfNotExistsOld-8   	     144	   8510219 ns/op	   18210 B/op	      40 allocs/op

Benchmark with 10 million keys (1.2 GB database):

BenchmarkCreateBucketIfNotExists-8      	     151	   7533890 ns/op	   18262 B/op	      40 allocs/op
BenchmarkCreateBucketIfNotExistsOld-8   	     154	   7600338 ns/op	   18282 B/op	      52 allocs/op

Benchmark code: 97e5461

[ahrtr]

Side effect: When the db is opened in readonly mode, the caller can get a valid bucket if present.

The existing implementation will return a ErrTxNotWritable error.

[ahrtr]

Suggest not to change the behavior.

[cenkalti]

Calling CreateBucketIfNotExists in read-only mode doesn't make sense. I don't think anybody relied on this behavior.

[ahrtr]

This PR creates a hole (readonly transaction can even create bucket (actually already exist) ) which might cause unnecessary confusion to users. It can even be regarded as a minor harmless CVE.

Please add the following code at the beginning of the method,

	if b.tx.db == nil {
		return nil, ErrTxClosed
	} else if !b.tx.writable {
		return nil, ErrTxNotWritable
	} else if len(key) == 0 {
		return nil, ErrBucketNameRequired
	}

[cenkalti]

Added.

[cenkalti]

Also added benchmark results. See PR description. It doesn't help with the operation speed but reduces allocations when there are many buckets which is a valid use cases where buckets are dynamically generated.

[ahrtr]

LGTM

Thanks

[ahrtr]

Just as #118 (comment) mentioned, it's a tradeoff. When it's highly likely the bucket doesn't exist, then CreateBucket firstly makes sense. But if it is highly likely the bucket exists, then your PR makes sense.

If you really want the PR makes sense for both cases, CreateBucketIfNotExists shouldn't reuse CreateBucket and Bucket directly; instead, it should reuse them in code level, something like below,

func (b *Bucket) CreateBucketIfNotExists(key []byte) (*Bucket, error) {
	if b.tx.db == nil {
		return nil, errors.ErrTxClosed
	} else if !b.tx.writable {
		return nil, errors.ErrTxNotWritable
	} else if len(key) == 0 {
		return nil, errors.ErrBucketNameRequired
	}

	if b.buckets != nil {
		if child := b.buckets[string(key)]; child != nil {
			return child, nil
		}
	}

	// Move cursor to correct position.
	c := b.Cursor()
	k, v, flags := c.seek(key)

	// Return an error if there is an existing non-bucket key.
	if bytes.Equal(key, k) {
		if (flags & common.BucketLeafFlag) != 0 {
			var child = b.openBucket(v)
			if b.buckets != nil {
				b.buckets[string(key)] = child
			}

			return child, nil
		}
		return nil, errors.ErrIncompatibleValue
	}

	// Create empty, inline bucket.
	var bucket = Bucket{
		InBucket:    &common.InBucket{},
		rootNode:    &node{isLeaf: true},
		FillPercent: DefaultFillPercent,
	}
	var value = bucket.write()

	// Insert into node.
	key = cloneBytes(key)
	c.node().put(key, key, value, 0, common.BucketLeafFlag)

	// Since subbuckets are not allowed on inline buckets, we need to
	// dereference the inline page, if it exists. This will cause the bucket
	// to be treated as a regular, non-inline bucket for the rest of the tx.
	b.page = nil

	return b.Bucket(key), nil
}

Attached the patch, feel free to apply the patch if it makes sense to you.
CreateBucketIfNotExists.txt

[ahrtr]

please update this PR per #504 (comment)

[cenkalti]

@ahrtr I think the patch adds too much code (+complexity) to gain little benefit. My first intention was to optimize the code for cases where a lot of buckets are automatically generated and kept around. CreateBucketIfNotExists is what I would use instead of writing:

b = findBucket()
if not b:
    b = createBucket()

So that's why I sent this PR. We should either accept the PR as is or reject and close the issue as wontfix.

[ahrtr]

My first intention was to optimize the code for cases where a lot of buckets are automatically generated and kept around.

Just as I mentioned, you PR only applies to one case. You resolved one "issue", but introduced another "issue". It isn't accepted.

@ahrtr I think the patch adds too much code (+complexity) to gain little benefit.

The point is we should do the right thing. Although it's a little complicated, it's still under control and should be accepted.

We should either accept the PR as is or reject and close the issue as wontfix.

Let's close this PR, and raise a new one with my patch.

[ahrtr]

FYI. #532