Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

backupccl: move alloc heavy Files field from manifest to SST #93997

Merged
merged 2 commits into from
Jan 10, 2023

Conversation

adityamaru
Copy link
Contributor

Repeated fields in a backup's manifest do not scale well as
the amount of backed up data, and the length of the incremental
chain of backup grows. This has been a problem we have been aware
of, and has motivated us to incrementally move all repeated fields
out of the manifest and into their standalone metadata SST files.
The advantage of this is that during incremental backups or restores
we do not need to perform large allocations when unmarshalling the
manifest, but instead stream results from the relevant SST as and
when we need them. In support issues such as
#93272 we have seen this
unmarshalling step results in OOMs thereby preventing further incremental
backups, or making the backups unrestoreable. Efforts for moving backup,
and restore to have all their metadata in SSTs and rely on streaming reads
and writes is ongoing but outside the scope of this patch.

This patch is meant to be a targetted fix with an eye for backports.
Past experimentation has shown us that the Files repeated field in the
manifest is the largest cause of bloated, unmarshalable manifests. This change
teaches backup to continue writing a manifest file, but a slimmer one
with the Files field nil'ed out. The values in the Files field are
instead written to an SST file that sits alongside the SLIM_BACKUP_MANIFEST.
To maintain mixed-version compatability with nodes that rely on a regular
manifest, we continue to write a BACKUP_MANIFEST alongside its slim version.
On the read path, we add an optimization that first reads the slim
manifest if present. This way we avoid unmarshalling the alloc heavy
Files field, and instead teach all the places in the code that need the
Files to reach out to the metadata SST and read the values one by one. To
support both the slim and not-so-slim manifests we introduce an interface
that iterates over the Files depending on the manifest passed to it.

To reiterate, this work is a subset of the improvements we will get
from moving all repeated fields to SSTs and is expected to be superseded
by those efforts when they come to fruition.

Fixes: #93272

Release note (performance improvement): long chains of incremental backups
and restore of such chains will now allocate less memory during the unmarshaling
of metadata

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@adityamaru adityamaru force-pushed the slim-manifest branch 2 times, most recently from a2d9f17 to 2539055 Compare December 21, 2022 04:35
@adityamaru adityamaru marked this pull request as ready for review December 21, 2022 05:42
@adityamaru adityamaru requested review from a team as code owners December 21, 2022 05:42
@adityamaru
Copy link
Contributor Author

friendly ping @rhu713 / @dt / @stevendanna for a first pass! we would like to get this in to master as soon as we can so that a backport to address https://github.com/cockroachlabs/support/issues/1939 can be opened

@benbardin
Copy link
Collaborator

Man, I don't love the additional backup manifest file. Rui, do I recall correctly that restore is already set up to read from the SST if the Files field isn't present? (It's been a while since we looked at this so I certainly might be recalling wrong.) If so, I think we could just remove Files from the regular manifest instead of adding the SLIM_ one.

@adityamaru
Copy link
Contributor Author

I think we could just remove Files from the regular manifest instead of adding the SLIM_ one

One reason to add a slim manifest is that writing the metadata.sst is not enabled on older branches (22.2 and 22.1), and master as of now. We defaulted to off because there are open issues such as #86806 that we need to iron out. So if we were to rely on the metadata.sst instead of the slim manifest then we'd need to enable that on older branches which is a riskier change with unknown unknowns.

do I recall correctly that restore is already set up to read from the SST if the Files field isn't present

My read of the code is that restore does not check for the existence of, or read the metadata.sst file today. The read path only looks for a manifest and passes the manifest proto around to the different call sites that need information from that manifest. I think this PR is an example of how we would eventually switch our read paths in manifest_handling.go to rely on the metadata.sst to resolve the manifest, and then have the specific SSTs containing repeated fields read at the different call sites.

Copy link
Member

@dt dt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly naming nits, one real question about the span of the poison pill entry

pkg/ccl/backupccl/backupinfo/backup_metadata.go Outdated Show resolved Hide resolved
pkg/ccl/backupccl/backupbase/constants.go Outdated Show resolved Hide resolved
pkg/ccl/backupccl/backuppb/backup.proto Outdated Show resolved Hide resolved
pkg/ccl/backupccl/backuppb/backup.proto Outdated Show resolved Hide resolved
pkg/ccl/backupccl/backupinfo/manifest_handling.go Outdated Show resolved Hide resolved
pkg/ccl/backupccl/backupinfo/manifest_handling.go Outdated Show resolved Hide resolved
pkg/ccl/backupccl/backupinfo/manifest_handling.go Outdated Show resolved Hide resolved
pkg/ccl/backupccl/backupinfo/manifest_handling.go Outdated Show resolved Hide resolved
pkg/ccl/backupccl/backupinfo/manifest_handling.go Outdated Show resolved Hide resolved
Repeated fields in a backup's manifest do not scale well as
the amount of backed up data, and the length of the incremental
chain of backup grows. This has been a problem we have been aware
of, and has motivated us to incrementally move all repeated fields
out of the manifest and into their standalone metadata SST files.
The advantage of this is that during incremental backups or restores
we do not need to perform large allocations when unmarshalling the
manifest, but instead stream results from the relevant SST as and
when we need them. In support issues such as
cockroachdb#93272 we have seen this
unmarshalling step results in OOMs thereby preventing further incremental
backups, or making the backups unrestoreable. Efforts for moving backup,
and restore to have all their metadata in SSTs and rely on streaming reads
and writes is ongoing but outside the scope of this patch.

This patch is meant to be a targetted fix with an eye for backports.
Past experimentation has shown us that the `Files` repeated field in the
manifest is the largest cause of bloated, unmarshalable manifests. This change
teaches backup to continue writing a manifest file, but a slimmer one
with the `Files` field nil'ed out. The values in the `Files` field are
instead written to an SST file that sits alongside the `SLIM_BACKUP_MANIFEST`.
To maintain mixed-version compatability with nodes that rely on a regular
manifest, we continue to write a `BACKUP_MANIFEST` alongside its slim version.
On the read path, we add an optimization that first reads the slim
manifest if present. This way we avoid unmarshalling the alloc heavy
`Files` field, and instead teach all the places in the code that need the
`Files` to reach out to the metadata SST and read the values one by one. To
support both the slim and not-so-slim manifests we introduce an interface
that iterates over the Files depending on the manifest passed to it.

To reiterate, this work is a subset of the improvements we will get
from moving all repeated fields to SSTs and is expected to be superseded
by those efforts when they come to fruition.

Fixes: cockroachdb#93272

Release note (performance improvement): long chains of incremental backups
and restore of such chains will now allocate less memory during the unmarshaling
of metadata
@adityamaru
Copy link
Contributor Author

bors r=dt,rhu713

@craig
Copy link
Contributor

craig bot commented Jan 10, 2023

Build succeeded:

@craig craig bot merged commit 4ada18c into cockroachdb:master Jan 10, 2023
@adityamaru
Copy link
Contributor Author

blathers backport 22.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

backupccl: write a slimmer manifest without alloc heavy repeated fields
5 participants