storageccl,backupccl: paginate ExportRequest to control response size

[adityamaru]

This change adds pagination support to ExportRequest's which return the
produced SSTs as part of their response. This is necessary to prevent
OOMs.

The motivation for this change was the work being done to support
tenants BACKUPs. Since all export requests will be returning the SST
from KV to the processor, instead of writing directly to
ExternalStorage, we will be incurring additional copying/buffering of
the produced SSTs. It is important to have a knob to control these
response sizes. The change piggybacks on the existing TargetBytes
functionality baked into the DistSender.

Fixes: #57227

Release note: None

[cockroach-teamcity]

This change is 

[adityamaru]

Will update the ReturnSST bool to use the flag once #57317 is merged. Will also add a tenant backup test to exercise the processor pagination when I update to using that flag.

[dt]

I know we have byte-size limited scans already so I wonder if there's a reason they didn't introduce this already, and/or if it will cause confusion if we introduce it here but don't use it there? /cc @nvanbenschoten

[nvanbenschoten]

That's a good question. We seem to be using the RESUME_KEY_LIMIT reason in places where this new RESUME_BYTE_LIMIT reason would be more appropriate, like:

cockroach/pkg/kv/kvclient/kvcoord/dist_sender.go

Line 1326 in 20f2a54

resumeReason = roachpb.RESUME_KEY_LIMIT

My best guess as to why we didn't do this in 9e11347 is because we would then have needed to plumb this reason back out of MVCC operations. We don't seem to use the reason for anything important (or am I missing something?), so maybe we didn't think it was worth the effort? I'm not totally sure, @tbg might know better.

[tbg]

Yes, I didn't see a strong enough reason to introduce it at the time.

PS: I don't have context on this PR.

[pbardea]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, @nvanbenschoten, and @pbardea)

---

pkg/ccl/backupccl/backup_processor.go, line 60 at r1 (raw file):

// resulting from buffering/copying large SST files.
// TODO(adityamaru): tune this via experimentation.
const ExportRequestSSTTargetBytes = 128<<20 /* 128 MiB */

It might be nice to have this as a cluster setting as an escape hatch against very large rows? I also think that this value will have an interesting relationship with ExportRequestTargetFileSize. I could see surprising behaviour if this one is set below the other. I think it would overshoot? But now I'm wondering if they should be one and the same setting.

---

pkg/roachpb/api.proto, line 74 at r1 (raw file):

Previously, dt (David Taylor) wrote…

I know we have byte-size limited scans already so I wonder if there's a reason they didn't introduce this already, and/or if it will cause confusion if we introduce it here but don't use it there? /cc @nvanbenschoten

I also shared this concern -- it might be easier to introduce the RESUME_BYTE_LIMIT separately?

[adityamaru]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, @nvanbenschoten, and @pbardea)

pkg/ccl/backupccl/backup_processor.go, line 60 at r1 (raw file):

// resulting from buffering/copying large SST files.
// TODO(adityamaru): tune this via experimentation.
const ExportRequestSSTTargetBytes = 128<<20 /* 128 MiB */

It might be nice to have this as a cluster setting as an escape hatch against very large rows? I also think that this value will have an interesting relationship with ExportRequestTargetFileSize. I could see surprising behaviour if this one is set below the other. I think it would overshoot? But now I'm wondering if they should be one and the same setting.

@pbardea I've been thinking about this and can't zero down on what the desired relationship between the two (or one) settings would be. If you set the response target size to less than the SSTTargetSize, the SST can definitely overshoot, because the response target size is not taken into account when creating the actual SST. So in the worst case you can get back a targetSST + overrage size SST even though you asked to cap off at something lower.

If you set the response target size to a factor of the SSTTargetSize, you could potentially fit multiple SSTs in a single response (depending on overrage). Lastly, if you merge the two into a single option, you will only be able to fit a single target size SST into the response every time. Is the ability to potentially send back multiple TargetSize SSTs in a single response, in other words not having to send a "resume" export request a big enough win that we keep two settings?

[pbardea]

I've been thinking about this and can't zero down on what the desired relationship between the two (or one) settings would be.

From my perspective, I think it makes the most sense to make this setting a cluster setting that specifies the export request SSTTargetByte size as a multiple of the other setting. That way changing ExportRequestTargetFileSize will not change the interaction between the settings, but they can each be individually tweaked. WDYT?

[adityamaru]

@pbardea this is RFAL. The new changes as per our discussion are:

• Always setting TargetBytes to sentinel value of 1, so that we paginate after a single SST in ExportSST. This allows the user to control the size of the SST returned in the response with the already existing target and overrage cluster settings. This approach also matches the pagination semantics at the pebble level which always creates a single SST based on the above-mentioned cluster settings before paginating.
• Progress is reported only one the entire span (all its ResumeSpans) have been processed. This is required for progress logging to work. A follow-up PR to cleanup how we do progress logging is on the way, but independent of this change.
• I had to add logic to take the min of the system tenant targetSize cluster setting, and the targetSize passed in from the tenant because we do not support cluster settings within a tenant yet. After some discussion with @dt there is some follow-up work to be done where we will allow the tenant to pass in its targetSize and have a maxSize cluster setting on the system tenant to control how much RAM is used.

[pbardea]

LGTM

[adityamaru]

TFTR!

bors r=pbardea

[craig]

Build succeeded:

• GitHub CI (Cockroach)