storage/storageccl: Add an option to break export mid key

[aliher1911]

Previously when exporting ranges with very large number of versions
export could hit the situation where single key export would exceed
maximum export byte limit. To resolve this, safety threshold has to
be raised which could cause nodes going out of memory.

To address this, this patch adds an ability to stop and resume on
the arbitrary key timestamp so that it could be stopped mid key and
resumed later.

Release note: None

Fixes #68231

[cockroach-teamcity]

This change is 

[aliher1911]

Want to get feedback on this before I add any client side code from backup_processor to the mix.
Currently split into 2 commits, one on the pebble side and another on request/response for export.

[pbardea]

Reviewed 2 of 9 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aliher1911 and @sumeerbhola)

---

pkg/ccl/storageccl/export.go, line 183 at r2 (raw file):

	// Only use resume timestamp if splitting mid key is enabled.
	resumeKeyTs := hlc.Timestamp{}
	if args.SplitMidKey {

Can we only permit SplitMidKey be set if args.ReturnSST is set? Backup/restore's accounting of these SSTs treats them as covering a complete span, so if we do split and ExportRequest mid SST, the backup processor should have a chance to stitch it together at the callsite. (We don't want to add a timestamp field to BackupManifest_File.)

We'll soon want to move to all requests setting args.ReturnSST to true so I don't think that this is a big limitation.

---

pkg/roachpb/api.proto, line 1368 at r2 (raw file):

  // different from start_time.
  util.hlc.Timestamp resume_key_ts = 12 [(gogoproto.nullable) = false];
  util.hlc.Timestamp start_time = 3 [(gogoproto.nullable) = false];

With serveral timestamps in the request, perhaps it'd be worthwhile to add a comment saying that StartTime is meaningful only with MVCCFilter set to all revisions, and that it is the lower bound on MVCCVersions of exported keys?

---

pkg/roachpb/api.proto, line 1445 at r2 (raw file):

  message File {
    Span span = 1 [(gogoproto.nullable) = false];
    util.hlc.Timestamp resume_key_ts = 9 [(gogoproto.nullable) = false];

Does resumeKeyTS semantically make sense to describe the contents of the file? Perhaps something like endKeyTS?

Also nit: (gogoproto.customname) = "ResumeKeyTS" (for here and the change to exportrequest above)

---

pkg/storage/pebble_batch.go, line 136 at r1 (raw file):

	startKey, endKey roachpb.Key,
	startTS, endTS hlc.Timestamp,
	firstKeyTs hlc.Timestamp,

nit: s/firstKeyTs/firstKeyTS/g and s/resumeTs/resumeTS/g

[aliher1911]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aliher1911, @pbardea, and @sumeerbhola)

---

pkg/ccl/storageccl/export.go, line 183 at r2 (raw file):

Previously, pbardea (Paul Bardea) wrote…

Can we only permit SplitMidKey be set if args.ReturnSST is set? Backup/restore's accounting of these SSTs treats them as covering a complete span, so if we do split and ExportRequest mid SST, the backup processor should have a chance to stitch it together at the callsite. (We don't want to add a timestamp field to BackupManifest_File.)

We'll soon want to move to all requests setting args.ReturnSST to true so I don't think that this is a big limitation.

I think we can't just remove the option because of mixed version clusters to retain ability to still do non-split exports, but we can check that this option can't be set if ReturnSST is false and fail.
Does that make sense?

[pbardea]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aliher1911 and @sumeerbhola)

---

pkg/ccl/storageccl/export.go, line 183 at r2 (raw file):

but we can check that this option can't be set if ReturnSST is false and fail

Yes, that sounds good

Sorry, I didn't meant to remove any option, just that we expect almost all requests (the exception being mixed-version requests) to set args.ReturnSST to true.

[sumeerbhola]

The storage changes look fine.

• MVCCIncrementalIterator.SeekGE has a code comment that says "startKey should be a metadata key to ensure ..." that needs tweaking.
• is the logic inpebbleExportToSst adequately tested using export_test.go -- could you look at the code coverage?

Reviewed 8 of 9 files at r1, 1 of 3 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aliher1911 and @sumeerbhola)

---

pkg/storage/engine.go, line 410 at r1 (raw file):

	//
	// This function looks at MVCC versions and intents, and returns an error if an
	// intent is found.

can you add stuff to the function comment about firstKeyTs and stopMidKey.

[aliher1911]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @aliher1911, @pbardea, and @sumeerbhola)

---

pkg/ccl/storageccl/export.go, line 183 at r2 (raw file):

Previously, pbardea (Paul Bardea) wrote…

but we can check that this option can't be set if ReturnSST is false and fail

Yes, that sounds good

Sorry, I didn't meant to remove any option, just that we expect almost all requests (the exception being mixed-version requests) to set args.ReturnSST to true.

Done.

---

pkg/roachpb/api.proto, line 1368 at r2 (raw file):

Previously, pbardea (Paul Bardea) wrote…

With serveral timestamps in the request, perhaps it'd be worthwhile to add a comment saying that StartTime is meaningful only with MVCCFilter set to all revisions, and that it is the lower bound on MVCCVersions of exported keys?

Done.

---

pkg/roachpb/api.proto, line 1445 at r2 (raw file):

resumeKeyTS
Yeah you are absolutely right! endKeyTS would be more consistent with how it is named in the Span where we have start and end keys.

[aliher1911]

@sumeerbhola can you elaborate on SeekGE, is it no longer the case that we can seek intents and keys separately because of how we store them now? Our current resume usage didn't change much here.

Test coverage look fine, I just added new tests there and wrapped that as a table test to cover different cases.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @pbardea and @sumeerbhola)

---

pkg/storage/engine.go, line 410 at r1 (raw file):

Previously, sumeerbhola wrote…

can you add stuff to the function comment about firstKeyTs and stopMidKey.

Seems like comment went a bit out of date since writer was added. Does it also make sense to wrap it in a struct later, since not all of them are always applicable.

[sumeerbhola]

The fact that they are separate is hidden from the user of MVCCIncrementalIterator, so I didn't mean to refer to the separated intents stuff. The comment currently says

startKey should be a metadata key to ensure that the iterator has a chance to observe any intents on the key if they are there.

and now we are not necessarily passing in a metadata key because we may have already observed the intent and are resuming from a particular version.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @pbardea and @sumeerbhola)

[aliher1911]

Updated comments on MVCCIncrementalIterator!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @pbardea and @sumeerbhola)

[sumeerbhola]

Reviewed 1 of 4 files at r8.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @aliher1911, @pbardea, and @sumeerbhola)

---

pkg/storage/engine.go, line 410 at r1 (raw file):

Previously, aliher1911 (Oleg) wrote…

Seems like comment went a bit out of date since writer was added. Does it also make sense to wrap it in a struct later, since not all of them are always applicable.

wrapping in an options struct sounds good.

---

pkg/storage/engine.go, line 394 at r8 (raw file):

	// If stopMidKey is used, firstKeyTS would serve as the timestamp for export to
	// resume together with a startKey. firstKeyTs doesn't necessarily need to be
	// between startTS and endTS.

what is the case in which it won't be in (startTS, endTS]?
also, it looks like firstKeyTS is unilaterally used. Maybe say that:
firstKeyTS is either empty, which represents starting from the potential intent and continuing to the versions, or non-empty, which represents starting from a particular version. firstKeyTS will always be empty when !stopMidKey.

[aliher1911]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @aliher1911, @pbardea, and @sumeerbhola)

---

pkg/storage/engine.go, line 394 at r8 (raw file):

Previously, sumeerbhola wrote…

what is the case in which it won't be in (startTS, endTS]?
also, it looks like firstKeyTS is unilaterally used. Maybe say that:
firstKeyTS is either empty, which represents starting from the potential intent and continuing to the versions, or non-empty, which represents starting from a particular version. firstKeyTS will always be empty when !stopMidKey.

Currently it is always empty or within (startTS, endTS] but there's another PR that would break this assumption if that change sounds feasible. Idea is to stop iterating after we hit certain underlying iteration limits #68467. In that case iterator could stop while skipping outside the (startTS, endTS] range and resuming it from that point will either bring us to start of interval or next key depending on where we stopped.

[aliher1911]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @pbardea and @sumeerbhola)

---

pkg/storage/engine.go, line 410 at r1 (raw file):

Previously, sumeerbhola wrote…

wrapping in an options struct sounds good.

I'll do that with separate PR

[pbardea]

Looks good to me

[aliher1911]

@sumeerbhola just checking if you have any more comments on that beside what was addressed. Before I rebase past breakages and merge that.

[aliher1911]

bors r=pbardea,sumeerbhola

[craig]

Build succeeded:

• GitHub CI (Cockroach)