libroach,engine: support pagination of ExportToSst

[ajwerner]

This commit extends the engine interface to take a targetSize parameter in
the ExportToSst method. The iteration will stope if the first version of a key
to be added to the SST would lead to targetSize being exceeded. If
exportAllRevisions is false, the targetSize will not be exceeded unless the
first kv pair exceeds it.

This commit additionally fixes a bug in the rocksdb implementation of
DBExportToSst whereby the first key in the export request would be skipped.
This case likely never occurred because the key passed to Export was rarely
exactly the first key to be included (see the change related to seek_key in
db.cc).

The exportccl.TestRandomKeyAndTimestampExport was extended to excercise various
targetSize limits. That test run under stress with the tee engine inspires some
confidence and did catch the above mentioned bug. More testing would likely be
good.

This commit leaves the task of adopting the targetSize parameter for later.

Fixes #39717.

Release note: None

[cockroach-teamcity]

This change is 

[ajwerner]

@itsbilal lmk if there's somebody better to review this. One improvement I'm considering is going through and naming the return param on the engine.Reader.ExportToSst method to make it more explicitly a resumeKey.

[petermattis]

I did a quick first pass and left a few comments on the C++. Not a full review.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @itsbilal)

---

c-deps/libroach/db.cc, line 1100 at r1 (raw file):

  // versions of a key to the writer.
  bool paginated = target_size > 0;
  std::string cur_key ("");

The explicit constructor call is unnecessary: s/ ("")//g.

---

c-deps/libroach/db.cc, line 1104 at r1 (raw file):

  // Seek to the MVCC metadata key for the provided start key and let the
  // incremental iterator find the appropriate version.
  DBKey seek_key = (DBKey){ .key = start.key };

Huh, this is some weird new C++ syntax. Are field initializers like that part of C++ now? I know for a while that was a gcc-extension. Doing some googling, it isn't clear to me if the (DBKey) cast is necessary. I think that can be removed. Maybe.

[ajwerner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @itsbilal and @petermattis)

---

c-deps/libroach/db.cc, line 1100 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

The explicit constructor call is unnecessary: s/ ("")//g.

Done.

---

c-deps/libroach/db.cc, line 1104 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Huh, this is some weird new C++ syntax. Are field initializers like that part of C++ now? I know for a while that was a gcc-extension. Doing some googling, it isn't clear to me if the (DBKey) cast is necessary. I think that can be removed. Maybe.

It’s kosher as of C99 so I assumed it would just work in C++. You’re right about the cast. Removed.

If there's a more idiomatic way of constructing the DBKey with a zero timestamp, let me know. In go we call MakeMVCCMetadataKey.

[petermattis]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @itsbilal)

---

c-deps/libroach/db.cc, line 1104 at r1 (raw file):

Previously, ajwerner wrote…

It’s kosher as of C99 so I assumed it would just work in C++. You’re right about the cast. Removed.

If there's a more idiomatic way of constructing the DBKey with a zero timestamp, let me know. In go we call MakeMVCCMetadataKey.

This idiom looks good to me.

---

c-deps/libroach/db.cc, line 1099 at r2 (raw file):

  // SST then we need to keep track of when we've finished adding all of the
  // versions of a key to the writer.
  bool paginated = target_size > 0;

Might want to sprinkle some const declarations around. Looks like this variable can be const, and some of the ones in the loop below.

[itsbilal]

save for some minor comments.

Reviewed 8 of 10 files at r1, 2 of 2 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)

---

c-deps/libroach/db.cc, line 1141 at r2 (raw file):

    int64_t new_size = cur_size + decoded_key.size() + iter.value().size();
    bool is_over_target = paginated && cur_size > 0 && new_size > target_size;
    if (is_new_key && is_over_target) {

Nit: Might be a good idea adding paginated && to this conditional and removing it from is_over_target. Makes it clearer that this only runs in the paginated cases, just like the conditional above. The boolean logic here (and the interactions between export_all_revisions, paginated, etc) are confusing enough.

---

c-deps/libroach/db.cc, line 1170 at r2 (raw file):

  // If we're not returning an error, check to see if we need to return the resume key.
  if (res.data == NULL && resume_key.length() > 0) {

Maybe s/res.data == NULL/res.ok()/ ? That's how we usually check for non-error DBStatuses.

---

c-deps/libroach/db.cc, line 1171 at r2 (raw file):

  // If we're not returning an error, check to see if we need to return the resume key.
  if (res.data == NULL && resume_key.length() > 0) {
    *resume = ToDBString(rocksdb::Slice(resume_key));

You should also be able to skip the resume_key conversion from string to rocksdb::Slice. ToDBString should still work. Rocksdb passes the two types around interchangeably a lot.

---

pkg/storage/engine/pebble.go, line 1153 at r2 (raw file):

		}
		unsafeValue := iter.UnsafeValue()
		isNewKey := !exportAllRevisions || unsafeKey.Key.Compare(curKey) != 0

bytes.Equal(unsafeKey.Key, curKey) is a bit more performant and easier to read

---

pkg/storage/engine/pebble.go, line 1169 at r2 (raw file):

			isOverTarget := paginated && curSize > 0 && uint64(newSize) > targetSize
			if isNewKey && isOverTarget {
				resumeKey = append(roachpb.Key{}, unsafeKey.Key...) // allocate the right size

Unsure of what the comment is referring to - do you want to allocate exactly len(unsafeKey.Key) bytes? In that case why not make([]byte, len(unsafeKey.Key)) and then copy(...) ?

[ajwerner]

Thanks for the prompt reviews!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner, @itsbilal, and @petermattis)

---

c-deps/libroach/db.cc, line 1099 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Might want to sprinkle some const declarations around. Looks like this variable can be const, and some of the ones in the loop below.

Done.

---

c-deps/libroach/db.cc, line 1121 at r2 (raw file):

    }

    bool is_new_key = !export_all_revisions || decoded_key.compare(rocksdb::Slice(cur_key)) != 0;

turns out I didn't need this rocksdb::Slice either.

---

c-deps/libroach/db.cc, line 1141 at r2 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

Nit: Might be a good idea adding paginated && to this conditional and removing it from is_over_target. Makes it clearer that this only runs in the paginated cases, just like the conditional above. The boolean logic here (and the interactions between export_all_revisions, paginated, etc) are confusing enough.

Done.

---

c-deps/libroach/db.cc, line 1170 at r2 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

Maybe s/res.data == NULL/res.ok()/ ? That's how we usually check for non-error DBStatuses.

erm that method seems to be on a rocksdb::Status this is just a DBStatus. The comment there says If DBStatus.data == NULL the operation succeeded. I could use status here but it's a little bit less. We also already do the same check above on status.

---

c-deps/libroach/db.cc, line 1171 at r2 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

You should also be able to skip the resume_key conversion from string to rocksdb::Slice. ToDBString should still work. Rocksdb passes the two types around interchangeably a lot.

Done.

---

pkg/storage/engine/pebble.go, line 1153 at r2 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

bytes.Equal(unsafeKey.Key, curKey) is a bit more performant and easier to read

ha forgot about Equals after the C++.

---

pkg/storage/engine/pebble.go, line 1169 at r2 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

Unsure of what the comment is referring to - do you want to allocate exactly len(unsafeKey.Key) bytes? In that case why not make([]byte, len(unsafeKey.Key)) and then copy(...) ?

See how this makes you feel.

[itsbilal]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @itsbilal, and @petermattis)

---

c-deps/libroach/db.cc, line 1170 at r2 (raw file):

Previously, ajwerner wrote…

erm that method seems to be on a rocksdb::Status this is just a DBStatus. The comment there says If DBStatus.data == NULL the operation succeeded. I could use status here but it's a little bit less. We also already do the same check above on status.

My bad, forgot this was DBStatus. Sounds good then.

---

pkg/storage/engine/pebble.go, line 1169 at r2 (raw file):

Previously, ajwerner wrote…

See how this makes you feel.

Looks better - letting append grow the slice from 0 usually means it'll end up being some power of 2. This is better if you want to allocate exactly that length.

[ajwerner]

bors r+

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @itsbilal, and @petermattis)

[craig]

Build succeeded

• GitHub CI (Cockroach)