teach pebble about block level prefix synthesis

[msbutler]

See individual commits

[cockroach-teamcity]

This change is 

[RaduBerinde]

Nice! This is so much simpler than the prefix iterator approach (which I'm still trying to get to work in all cases)..

We should add checks in NewRawRangeDelIter and NewRawRangeKeyIter and error out since we can't handle the transform there.

Reviewable status: 0 of 10 files reviewed, 5 unresolved discussions (waiting on @msbutler)

---

-- commits line 10 at r1:
This slowdown is when prefix synthesis is not enabled, correct?

---

sstable/block.go line 774 at r1 (raw file):

	if i.transforms.SyntheticPrefix != nil {
		if !bytes.HasPrefix(key, i.transforms.SyntheticPrefix) {
			if i.cmp(i.transforms.SyntheticPrefix, key) >= 0 {

This is a problem - cmp is only defined for full keys. The CRDB comparer looks at the last byte and treats it as a suffix length so things would break.
What the prefix replacing iterator does is to also pass a "test" key in the synthetic prefix range (I use the vtable lower bound) and use that for the comparison.

---

sstable/reader.go line 173 at r1 (raw file):

}

// SyntheticPrefix represents a byte slice that it implicitly prepended to every

*is

---

sstable/reader.go line 175 at r1 (raw file):

// SyntheticPrefix represents a byte slice that it implicitly prepended to every
// key in a file being read or accessed by a reader. Specifically, the file is
// implied to contain keys that do not explicitly include the prefix, and the

This explanation is a bit confusing.. maybe say that the table is assumed to contain pieces of keys that become full keys when prepended a valid prefix. The table's bloom filters are constructed only on the pieces of keys in the table, but interactions with the file ...

One question I have - are the keys / "key pieces" in the table required to be full keys (i.e. can they be passed to Compare or Split?). I believe they will be in practice but we don't depend on that as long as they're valid with the SyntheticPrefix?

---

sstable/reader_common.go line 90 at r2 (raw file):

		return false
	}
	if p.ContentPrefix != nil && p.SyntheticPrefix != nil {

I'd compare lengths just in case. I think it's enough to check ContentPrefix

[msbutler]

Added those invariants.

Reviewable status: 0 of 10 files reviewed, 2 unresolved discussions (waiting on @msbutler and @RaduBerinde)

---

-- commits line 10 at r1:

Previously, RaduBerinde wrote…

This slowdown is when prefix synthesis is not enabled, correct?

Indeed. Amended the commit msg to clarify

---

sstable/block.go line 774 at r1 (raw file):

Previously, RaduBerinde wrote…

This is a problem - cmp is only defined for full keys. The CRDB comparer looks at the last byte and treats it as a suffix length so things would break.
What the prefix replacing iterator does is to also pass a "test" key in the synthetic prefix range (I use the vtable lower bound) and use that for the comparison.

Changed it to use i.FirstUserKey which gets hydrated at init.

I believe we could also use bytes.Compare() since the syntheticPrefix will never contain suffix bytes (which have weird ordering), but I think the FirstUserKey approach is cleaner.

---

sstable/reader.go line 175 at r1 (raw file):

Previously, RaduBerinde wrote…

This explanation is a bit confusing.. maybe say that the table is assumed to contain pieces of keys that become full keys when prepended a valid prefix. The table's bloom filters are constructed only on the pieces of keys in the table, but interactions with the file ...

One question I have - are the keys / "key pieces" in the table required to be full keys (i.e. can they be passed to Compare or Split?). I believe they will be in practice but we don't depend on that as long as they're valid with the SyntheticPrefix?

Done. Since a prefix-less key will always contain a full suffix, I believe all compare operations will be valid on it. Might be worth adding some invariants here.

[RaduBerinde]

Reviewable status: 0 of 11 files reviewed, 3 unresolved discussions (waiting on @msbutler)

---

sstable/block.go line 774 at r1 (raw file):

Previously, msbutler (Michael Butler) wrote…

Changed it to use i.FirstUserKey which gets hydrated at init.

I believe we could also use bytes.Compare() since the syntheticPrefix will never contain suffix bytes (which have weird ordering), but I think the FirstUserKey approach is cleaner.

This deserves a comment. Something like:

The seek key is before or after the entire range of keys that start with SyntheticPrefix. To determine which, we need to compare against a valid key in this range. We use firstUserKey which has the synthetic prefix.

---

sstable/reader_common.go line 90 at r5 (raw file):

		return false
	}
	if len(p.ContentPrefix)>0{

[nit] formatting (do you have crlfmt or gofmt set up to run automatically?)

---

testdata/ingest_external line 525 at r5 (raw file):

----

# Synthesize prefix with one greater than the original one.

These comments don't make sense, we are replacing the empty prefix in both cases.

[msbutler]

Reviewable status: 0 of 11 files reviewed, 2 unresolved discussions (waiting on @msbutler and @RaduBerinde)

---

sstable/reader_common.go line 90 at r5 (raw file):

Previously, RaduBerinde wrote…

[nit] formatting (do you have crlfmt or gofmt set up to run automatically?)

i've been running it manually with the following cmd:

❯ which mbfmt
mbfmt () {
	echo crlfmt
	git diff-tree --no-commit-id --name-only -r HEAD | grep --color=auto --exclude-dir={.bzr,CVS,.git,.hg,.svn,.idea,.tox} '.go' | xargs -n1 crlfmt -tab 2 -w
}

it's not great with multi commit prs. I should revisit running it automatically via my IDE.

---

testdata/ingest_external line 525 at r5 (raw file):

Previously, RaduBerinde wrote…

These comments don't make sense, we are replacing the empty prefix in both cases.

Oops, bad copy pasta.

[msbutler]

@RaduBerinde @sumeerbhola friendly ping to review this!

@sumeerbhola let me know if I should wait to merge until you have taken a look at this.

[RaduBerinde]

Awesome!

Reviewable status: 0 of 11 files reviewed, 7 unresolved discussions (waiting on @msbutler and @sumeerbhola)

---

sstable/block.go line 420 at r7 (raw file):

	// be used after ikey is set.
	key []byte
	// fullKey is a buffer used for key prefix decompression.

Mention that when a synthetic prefix is used, this always starts with that prefix.

---

sstable/block.go line 691 at r7 (raw file):

	}
	if i.transforms.SyntheticPrefix != nil {
		i.firstUserKey = append(append(i.firstUserKeyWithPrefixBuf[:0], i.transforms.SyntheticPrefix...), i.firstUserKey...)

[nit] this might allocate twice (not sure if Go "merges" the appends into a single operation). More importantly, we never actually set firstUserKeyWithPrefixBuf to anything, so we won't reuse buffers.

I'd replace with:

i.firstUserKeyWithPrefixBuf = slices.Grow(i.firstUserKeyWithPrefixBuf[:0], len(i.transforms.SyntheticPrefix) + len(i.firstUserKey))
i.firstUserKeyWithPrefixBuf = append(i.firstUserKeyWithPrefixBuf, i.transforms.SyntheticPrefix...)
i.firstUserKeyWithPrefixBuf = append(i.firstUserKeyWithPrefixBuf, i.firstUserKey...)
i.firstUserKey = i.firstUserKeyWithPrefixBuf

---

sstable/block.go line 964 at r7 (raw file):

				return i.Last()
			}
			i.offset = -1

Worth adding a comment like above, mentioning that Next will return the first user key.

---

sstable/reader_common.go line 86 at r9 (raw file):

// wrapper should be used.
func (p *PrefixReplacement) UsePrefixReplacementIterator() bool {
	if p == nil {

[nit] return p != nil && len(p.ContentPrefix) > 0

---

sstable/reader_test.go line 1381 at r9 (raw file):

					}
					if prefix != nil {
						keyToWrite = append(keyToWrite, prefix...)

[nit] keyToWrite = append(slices.Clip(prefix), key...)

[msbutler]

Reviewable status: 0 of 11 files reviewed, 2 unresolved discussions (waiting on @RaduBerinde and @sumeerbhola)

---

sstable/block.go line 420 at r7 (raw file):

Previously, RaduBerinde wrote…

Mention that when a synthetic prefix is used, this always starts with that prefix.

Done.

---

sstable/block.go line 691 at r7 (raw file):

Previously, RaduBerinde wrote…

[nit] this might allocate twice (not sure if Go "merges" the appends into a single operation). More importantly, we never actually set firstUserKeyWithPrefixBuf to anything, so we won't reuse buffers.

I'd replace with:

i.firstUserKeyWithPrefixBuf = slices.Grow(i.firstUserKeyWithPrefixBuf[:0], len(i.transforms.SyntheticPrefix) + len(i.firstUserKey))
i.firstUserKeyWithPrefixBuf = append(i.firstUserKeyWithPrefixBuf, i.transforms.SyntheticPrefix...)
i.firstUserKeyWithPrefixBuf = append(i.firstUserKeyWithPrefixBuf, i.firstUserKey...)
i.firstUserKey = i.firstUserKeyWithPrefixBuf

Done.

To clarify, this change will increase the capacity of firstUserKeyWithPrefixBuf, if necessary. But we do reuse the buf when we reset the iter for reuse, right?
https://github.com/msbutler/pebble/blob/butler-rebased-prefix/sstable/block.go#L541

---

sstable/block.go line 964 at r7 (raw file):

Previously, RaduBerinde wrote…

Worth adding a comment like above, mentioning that Next will return the first user key.

Done.

---

sstable/reader_common.go line 86 at r9 (raw file):

Previously, RaduBerinde wrote…

[nit] return p != nil && len(p.ContentPrefix) > 0

Done.

---

sstable/reader_test.go line 1381 at r9 (raw file):

Previously, RaduBerinde wrote…

[nit] keyToWrite = append(slices.Clip(prefix), key...)

Done.

[RaduBerinde]

Reviewable status: 0 of 11 files reviewed, all discussions resolved (waiting on @sumeerbhola)

---

sstable/block.go line 691 at r7 (raw file):

Previously, msbutler (Michael Butler) wrote…

Done.

To clarify, this change will increase the capacity of firstUserKeyWithPrefixBuf, if necessary. But we do reuse the buf when we reset the iter for reuse, right?
https://github.com/msbutler/pebble/blob/butler-rebased-prefix/sstable/block.go#L541

Right. I was saying that the previous version of the change (r7 on reviewable) did not ever set firstUserKeyWithPrefixBuf to anything, so it would always be nil

[sumeerbhola]

I did a quick pass, and have only a few nits

Reviewed 5 of 10 files at r10, 1 of 1 files at r11, 2 of 9 files at r12, all commit messages.
Reviewable status: 3 of 11 files reviewed, 4 unresolved discussions (waiting on @msbutler)

---

-- commits line 72 at r12:
different than ...

isn't this the buggy path (that @RaduBerinde was trying to fix in #3344) that we are abandoning in favor of that PR?

---

sstable/block.go line 783 at r10 (raw file):

				// with SyntheticPrefix. To determine which, we need to compare against a
				// valid key in the block. We use firstUserKey which has the synthetic
				// prefix.

nit: a comment like this belongs before the predicate, since that is the line of code this is talking about.

---

sstable/block.go line 965 at r10 (raw file):

			// with SyntheticPrefix. To determine which, we need to compare against a
			// valid key in the block. We use firstUserKey which has the synthetic
			// prefix.

same nit

---

sstable/reader.go line 181 at r10 (raw file):

// keys that did include the prefix. Note that all Compare operations may act on
// a prefix-less key as the synthetic prefix will never modify key metadata
// stored in the key suffix.

Should this also say that such files must not contain rangedels or range keys, since the transformation is only applied to the point keys.

[RaduBerinde]

-- commits line 72 at r12:

Previously, sumeerbhola wrote…

different than ...

isn't this the buggy path (that @RaduBerinde was trying to fix in #3344) that we are abandoning in favor of that PR?

Yes, the plan is to remove that once Restore is able to generate "prefix free" ssts.

[msbutler]

Reviewable status: 3 of 11 files reviewed, 3 unresolved discussions (waiting on @sumeerbhola)

---

sstable/block.go line 965 at r10 (raw file):

Previously, sumeerbhola wrote…

same nit

Done.

---

sstable/reader.go line 181 at r10 (raw file):

Previously, sumeerbhola wrote…

Should this also say that such files must not contain rangedels or range keys, since the transformation is only applied to the point keys.

Done.