storageccl: don't clear existing data in AddSSTable

[danhhz]

Any given AddSSTable command needs to be idempotent, so we can retry it
(in the face of AmbiguousResultError, etc). Previously, this was
accomplished by clearing the request span before ingest. This meant that
when the request completed successfully, the affected span contained
exactly the data in the payload sstable.

Instead, ingest the file and adjust MVCCStats as appropriate. Motivations:

• An unlikely race condition in the upcoming resumable RESTORE
  coordinator work. It will be possible that one half of a split-brain
  RESTORE will finish and expose the table publicly for writes before
  the other half gets off one last AddSSTable. This would destroy user
  data in a subtle, non-transactional way.

• Adding a range tombstone basically guarantees that the ingested
  sstable will be placed in L0 instead of some lower level, increasing
  read amplification.

[danhhz]

The first commit is in another PR and I need to rewrite TestAddSSTableMVCCStats but the rest of this is ready for review.

[cockroach-teamcity]

This change is 

[bdarnell]

---

Review status: 0 of 7 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[tbg]

---

Reviewed 2 of 2 files at r1, 2 of 2 files at r2, 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

pkg/ccl/storageccl/add_sstable.go, line 49 at r3 (raw file):

	// being ingested can overwrite all, some, or none of the existing kvs.
	// (Note: the expected case is that it's none or, in the case of a retry of
	// the request, all.) So subtract out the existing mvcc stats, and add back

Misplaced ..

---

pkg/ccl/storageccl/add_sstable.go, line 59 at r3 (raw file):

		log.Eventf(ctx, "target key range not empty, will merge existing data with sstable")
	}
	existingStats, err := existingIter.ComputeStats(mvccStartKey, mvccEndKey, h.Timestamp.WallTime)

This is cheap when there isn't any data, right?

---

pkg/ccl/storageccl/add_sstable.go, line 116 at r3 (raw file):

	}

	mergedIter := engineccl.MakeMultiIterator([]engine.SimpleIterator{existingIter, dataIter})

Perhaps add a comment about the importance of the ordering {existingIter, dataIter}.

---

pkg/ccl/storageccl/writebatch.go, line 83 at r3 (raw file):

}

func clearExistingData(

Is this still called?

---

pkg/ccl/storageccl/engineccl/multi_iterator.go, line 29 at r2 (raw file):

	// The indexes of every iterator with the same key and timestamp as the one
	// in currentIdx.
	itersWithCurrentKeyTimestamp []int

Not suggesting that you necessarily should, but could you remove currentIdx and just make that the last entry in itersWithCurrentKeyTimestamp?

---

Comments from Reviewable

[danhhz]

Thanks for the reviews! Finished the test. RFAL

---

Review status: 2 of 7 files reviewed at latest revision, 5 unresolved discussions, some commit checks pending.

---

pkg/ccl/storageccl/add_sstable.go, line 49 at r3 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Misplaced ..

Really? Where? Why?

---

pkg/ccl/storageccl/add_sstable.go, line 59 at r3 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

This is cheap when there isn't any data, right?

Added a note

---

pkg/ccl/storageccl/add_sstable.go, line 116 at r3 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Perhaps add a comment about the importance of the ordering {existingIter, dataIter}.

Done.

---

pkg/ccl/storageccl/writebatch.go, line 83 at r3 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Is this still called?

Yeah, in WriteBatch. I haven't worked through how the migration for it would work (and won't before 1.1)

---

pkg/ccl/storageccl/engineccl/multi_iterator.go, line 29 at r2 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Not suggesting that you necessarily should, but could you remove currentIdx and just make that the last entry in itersWithCurrentKeyTimestamp?

I think that would work

---

Comments from Reviewable

[tbg]

Reviewed 3 of 3 files at r4, 5 of 5 files at r5.
Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

pkg/ccl/storageccl/add_sstable.go, line 49 at r3 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

Really? Where? Why?

The comment isn't here any more.

---

pkg/ccl/storageccl/add_sstable_test.go, line 195 at r5 (raw file):

func TestAddSSTableMVCCStats(t *testing.T) {
	defer leaktest.AfterTest(t)()
	rng, _ := randutil.NewPseudoRand()

Probably want to log the seed because if it fails, then what?

---

pkg/ccl/storageccl/add_sstable_test.go, line 236 at r5 (raw file):

		var sstBytes []byte
		{

Make this func() { }() so that the defer within fires more coherently.

---

pkg/ccl/storageccl/writebatch.go, line 83 at r3 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

Yeah, in WriteBatch. I haven't worked through how the migration for it would work (and won't before 1.1)

Gotcha. Seems like you might be able to toss this out for free when 1.1->1.2 happens (I suppose we don't allow upgrades from 1.0 to 1.2).

---

Comments from Reviewable

[bdarnell]

Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

pkg/ccl/storageccl/writebatch.go, line 83 at r3 (raw file):

(I suppose we don't allow upgrades from 1.0 to 1.2)

Correct. We'll require that you are running 1.1 and have set the cluster's "use version" setting (or whatever that ends up being called) to 1.1 before beginning the process to upgrade to 1.2.

---

Comments from Reviewable

[danhhz]

Review status: 6 of 7 files reviewed at latest revision, 4 unresolved discussions.

---

pkg/ccl/storageccl/add_sstable_test.go, line 195 at r5 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Probably want to log the seed because if it fails, then what?

Whoops, yep. Meant to do this and forgot. Thanks

---

pkg/ccl/storageccl/add_sstable_test.go, line 236 at r5 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Make this func() { }() so that the defer within fires more coherently.

Done.

---

Comments from Reviewable