metamorphic: add key manager abstraction and SetWithDelete bug fix

[nicktrav]

Currently, the metamorphic test generator tracks the state of keys that
it generates to determine whether or not certain keys can be used for
specific operations. Specifically, keys that have been set once in the
database are eligible to be SingleDeleted. Future enhancements to the
test suite will likely introduce other desirable sequences of operations
that rely on tracking the state of certain keys through the metamorphic
test.

State tracking for keys is currently embedded directory onto the
generator struct. Additional fields that are to be used for state
tracking would continue to complicate the already sizable struct. There
is also significant cognitive overhead with the existing state tracking
for keys.

This commit introduces the concept of a "key manager", dedicated to
tracking the state of keys during metamorphic test generation time.
Internally, the keyManager struct can be passed operations via
ops (e.g. setOp, etc.) that transform the internal state. The
keyManager then exposes functions that can be used to deduce which
keys are eligible for certain operations at a point in time.

As part of this change, we split the paths that are asking for keys
for reads, writes (excluding SingleDelete), SingleDelete writes.

We can now generate complex sequences involving SingleDelete, which
lead to dicovering a bug where the code diverged from the solution
outlined in the issue). The following sequence
SET => (SINGLEDEL => SET) => SINGLEDEL
could be transformed to
SET => (SET => SINGLEDEL)
to
SET
which is incorrect. compactionIter contains the fix, and there is
a datadriven test that exercises this behavior. There are also
datadriven test cases that exercise some paths caused by snapshots.

One limitation of the keyManager is that since it used in the
generation phase it cannot account for operations that fail during
execution. We have one case of that involving multiple batches
being ingested. A failed operation can cause the keyManager to no
longer have the correct state of what will actually happen at runtime.
Which means a sequence of operations it generates could be invalid
and cause false positive test failures, since the state of different
DBs is allowed to be non-deterministic for these invalid sequences.
We have removed the troublesome failure cases to address this.

Informs #1255

[cockroach-teamcity]

This change is 

[nicktrav]

Note that there are some algorithmic improvements that can be made to the keyManager (have left some TODOs in the code). I didn't want to burn to much time on these, as we only use the key manager once per meta test run (when generating operations).

I also ran the metamorphic tests for ~1 hour just to be certain we weren't regressing. I've also spot checked some generated output to make sure they made senes w.r.t. SingleDel.

[sumeerbhola]

I'm adding changes as a second commit so that it is easier to see what I am changing. It won't build right now, since I've broken the test.

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

[sumeerbhola]

This is ready for a review:

• I've fixed the test, and added a bug fix, in the second commit.
• These will get squashed before merge, but I am keeping them separate for now. For reviewing purposes it is fine to review them combined.
• There is a metamorphic test failure related to SingleDelete that I am still in the process of diagnosing.

[itsbilal]

nice catch on the bug! good that this test is already catching things.

Reviewed 1 of 4 files at r1, 1 of 2 files at r2, 6 of 6 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jbowens and @nicktrav)

---

internal/metamorphic/key_manager.go, line 96 at r3 (raw file):

	// same *keyMeta as in the byObj slices. Using a map allows for fast state
	// lookups when changing the state based on a writer operation on the key.
	byObjKey map[string]*keyMeta

Any particular reason to use strings here? We could just use objKey as it should be serializable, and is safer as it would avoid misuse of a string not generated from objKey.String being used as a key.

---

internal/metamorphic/key_manager.go, line 108 at r3 (raw file):

	// writers, including inflight state that has not made its way to the DB
	// yet.The keyMeta.objKey is uninitialized.
	globalKeysMap map[string]*keyMeta

Same here

---

internal/metamorphic/key_manager_test.go, line 402 at r3 (raw file):

			},
		},
		// TODO(nicktrav): more cases

Probably a good idea to add a "set, del on db, set, singledel on batch" case, and a "set on db, singledel on batch" case.

---

internal/metamorphic/key_manager_test.go, line 422 at r3 (raw file):

}

// TODO(nicktrav): delete after adding more cases above.

can be deleted

[sumeerbhola]

TFTR!

The metamorphic test failure that I was diagnosing turned out to be a test problem -- we were randomizing the FormatMajorVersion to a value before these new SingleDelete semantics. I've changed the metamorphic test. This does mean that we do not have coverage for earlier versions that did not use the marker file, but given that this test is about testing operations and not really affected by how we use the marker instead of the CURRENT file, I think this is acceptable.

Reviewable status: 7 of 12 files reviewed, 4 unresolved discussions (waiting on @itsbilal, @jbowens, and @nicktrav)

---

internal/metamorphic/key_manager_test.go, line 402 at r3 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

Probably a good idea to add a "set, del on db, set, singledel on batch" case, and a "set on db, singledel on batch" case.

Done

---

internal/metamorphic/key_manager_test.go, line 422 at r3 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

can be deleted

Done

[sumeerbhola]

Reviewable status: 7 of 12 files reviewed, 4 unresolved discussions (waiting on @itsbilal and @jbowens)

---

internal/metamorphic/key_manager.go, line 96 at r3 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

Any particular reason to use strings here? We could just use objKey as it should be serializable, and is safer as it would avoid misuse of a string not generated from objKey.String being used as a key.

(reviewable ate my response, retrying)
there is a byte sllice in that struct, which means it can't be used as a map key.

---

internal/metamorphic/key_manager.go, line 108 at r3 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

Same here

ditto

[itsbilal]

Reviewed 5 of 5 files at r4, 5 of 5 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jbowens)