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: elide expensive ShowCreate call in SHOW BACKUP #88293

Merged
merged 1 commit into from Sep 23, 2022

Conversation

adityamaru
Copy link
Contributor

@adityamaru adityamaru commented Sep 20, 2022

In #88376 we see the call to ShowCreate taking ~all the time on a cluster with
2.5K empty tables. In all cases except SHOW BACKUP SCHEMAS we do not
need to construct the SQL representation of the table's schema. This
results in a marked improvement in the performance of SHOW BACKUP
as can be seen in #88376 (comment).

Fixes: #88376

Release note (performance improvement): SHOW BACKUP on a backup containing
several table descriptors is now more performant

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@adityamaru adityamaru marked this pull request as ready for review September 21, 2022 20:35
@adityamaru adityamaru requested a review from a team as a code owner September 21, 2022 20:35
@adityamaru adityamaru requested a review from a team September 21, 2022 20:35
@adityamaru adityamaru requested a review from a team as a code owner September 21, 2022 20:35
@adityamaru adityamaru changed the title [WIP,DNM] backupccl: identify SHOW BACKUP bottleneck backupccl: elide expensive ShowCreate call in SHOW BACKUP Sep 21, 2022
@adityamaru adityamaru added backport-21.2.x 21.2 is EOL backport-22.1.x 22.1 is EOL backport-22.2.x Flags PRs that need to be backported to 22.2. labels Sep 21, 2022
@adityamaru adityamaru removed request for a team September 21, 2022 21:01
@postamar
Copy link
Contributor

This looks promising.

In cockroachdb#88376 we see this call taking ~all the time on a cluster with
2.5K empty tables. In all cases except `SHOW BACKUP SCHEMAS` we do not
need to construct the SQL representation of the table's schema. This
results in a marked improvement in the performance of `SHOW BACKUP`
as can be seen in cockroachdb#88376 (comment).

Fixes: cockroachdb#88376

Release note (performance improvement): `SHOW BACKUP` on a backup containing
several table descriptors is now more performant
@adityamaru
Copy link
Contributor Author

TFTR!

bors r=stevendanna

@craig
Copy link
Contributor

craig bot commented Sep 23, 2022

Build succeeded:

@craig craig bot merged commit 2c18ed3 into cockroachdb:master Sep 23, 2022
@blathers-crl
Copy link

blathers-crl bot commented Sep 23, 2022

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from cc68174 to blathers/backport-release-21.2-88293: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 21.2.x failed. See errors above.


error creating merge commit from cc68174 to blathers/backport-release-22.1-88293: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.


error creating merge commit from cc68174 to blathers/backport-release-22.2-88293: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@postamar
Copy link
Contributor

Did we really want to merge all these tracing.ChildSpan calls in this PR? These hardly seem desirable.

@adityamaru
Copy link
Contributor Author

Did we really want to merge all these tracing.ChildSpan calls in this PR?

@postamar On the DR team we're consciously sprinkling these childspans in every method / sub-operation invoked during a job so that it surfaces more information when grabbing a job trace. In particular, we aggregate the time we spend in each childspan so that when a customer comes to us with a support escalation saying "why is my restore/backup slow?" we have a pretty good first step to narrow down on the cause of the slowdown. As #88376 traces can be quite useful in expediently finding the expensive call. Andrei and obs-inf have done a lot of work to make childspans very cheap to use. Do you have any concerns about this general direction we're headed in that I am missing?

@postamar
Copy link
Contributor

postamar commented Oct 3, 2022

Thanks for this explanation! I was assuming that they were left in by mistake. I'm surprised you can't use reflection to auto-generate the span name, wouldn't that be preferable?

adityamaru added a commit to adityamaru/cockroach that referenced this pull request Dec 6, 2022
Backport 1/1 commits from cockroachdb#88293.

/cc @cockroachdb/release

In cockroachdb#88376 we see the call to ShowCreate taking ~all the time on a cluster with
2.5K empty tables. In all cases except SHOW BACKUP SCHEMAS we do not
need to construct the SQL representation of the table's schema. This
results in a marked improvement in the performance of SHOW BACKUP
as can be seen in cockroachdb#88376 (comment).

Fixes: cockroachdb#88376

Release note (performance improvement): SHOW BACKUP on a backup containing
several table descriptors is now more performant

Release justification: low risk performance improvement required for the use of schedules in CockroachCloud
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-21.2.x 21.2 is EOL backport-22.1.x 22.1 is EOL backport-22.2.x Flags PRs that need to be backported to 22.2.
Projects
None yet
4 participants