db: add SharedLowerUserKeyPrefix and WriteSharedWithStrictObsolete op…

[sumeerbhola]

…tions

SharedLowerUserKeyPrefix, if specified, is an additional lower bound on constraint on key prefixes that should be written to shared files. It applies only when CreateOnShared permits some shared file creation. It will be used by CockroachDB to exclude keys below TableDataMin from shared files, for both correctness (they can contain MERGEs for which the obsolete bit does not work) and performance reasons (low latency is more important and the data volume is tiny).

WriteSharedWithStrictObsolete, when true, causes shared files to be written with WriterOptions.IsStrictObsolete set to true. This adds an extra measure of configuration protection to accidentally sharing files where the MERGE could become visible (we currently share such files, but file virtualization hides these MERGEs).

The enforcement of SharedLowerUserKeyPrefix changes how PreferSharedStorage is computed during flushes and compactions. It will only be set if the next key to be written by compact.Runner permits writing to shared storage. compact.Runner optimizes this computation for when a compaction is fully within the shared or unshared bounds. Additionally compact.Runner uses the existing OutputSplitter to decide when a split should happen when transitioning from unshared to shared.

While here, we do a tiny optimization in OutputSplitter to remove a key comparison on each iteration key.

Fixes #2756

[cockroach-teamcity]

This change is 

[sumeerbhola]

There is no test -- I'll add one once the configuration settings look ok.

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

[RaduBerinde]

LGTM, just some minor questions

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

---

db.go line 1261 at r1 (raw file):

//     can of course conform to the stronger remote.CreateOnSharedAll).
//
//   - If Options.Experimental.SharedLowerUserKeyPrefix is non-nil, the lower

Why not make this a generic user key rather than a prefix? We will still set it to a prefix in CRDB but it will make the boundary checks easier.

---

internal/compact/run.go line 110 at r1 (raw file):

	split base.Split
	// At most one state transition from false => true, when the next key should
	// be written to shared storage.

[nit] I'd point to cfg.SharedLowerUserKeyPrefix so the reader can understand the transition

---

internal/compact/run.go line 142 at r1 (raw file):

		r.cfg.SharedLowerUserKeyPrefix = nil
	} else {
		// Check if the compaction will only write non-shared keys.

Can't we just check compactionBounds.End.IsUpperBoundFor(SharedLowerUserKeyPrefix)? Are you worried of an end key that has the SharedLowerUserKeyPrefix and a suffix and yet it compares before the prefix? (i guess that's allowed even if it won't happen with crdb?)

---

internal/compact/splitting.go line 265 at r1 (raw file):

		// When the target file size limit is very small (in tests), we could end up
		// splitting at the first key, which is not allowed.
		if !s.pastStartKey && s.cmp(nextUserKey, s.startKey) <= 0 {

The change looks good but just to defend the existing code :) - this is not a performance-sensitive path: if we are here, we are not at a grandparent boundary and shouldSplitBasedOnSize said that we should split (and it will continue to do so). So the comparison only happens near the split boundaries.