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

release-23.1: sql/catalog: avoid fetching descriptors when fetching comments #103331

Merged
merged 2 commits into from
May 16, 2023

Conversation

blathers-crl[bot]
Copy link

@blathers-crl blathers-crl bot commented May 15, 2023

Backport 2/2 commits from #103106 on behalf of @fqazi.

/cc @cockroachdb/release


Previously, we refactored the code to fetch comments, descriptors, and zone config together in all cases. A side effect of this change was that the crdb_internal.kv_system_comments table was substantially slower for larger tables leading to big regressions. To address this, this patch adds a method of only fetching comments within the collections.

Additionally, we added many builtins into both functions, which meant our existing virtual index look-up could end up falling back too frequently to full scans. Also, a virtual index is added on the kv_catalog_comments table for fast point look-ups when referencing descriptors.

Informs: #102851
Fixes: #102613

Release note (bug fix): Optimize over-head of
pg_catalog.pg_description and pg_catalog.pg_shdescription, which can lead to performance regression relative to 22.2


Release justification: necessary performance enhancement

Previously, we refactored the code to fetch comments, descriptors,
and zone config together in all cases. A side effect of this change
was that the crdb_internal.kv_system_comments table was substantially
slower for larger tables leading to big regressions. To address this,
this patch adds a method of only fetching comments within the
collections.

Informs: #102851
Fixes: #102613

Release note (bug fix): Optimize over-head of
pg_catalog.pg_description and pg_catalog.pg_shdescription, which
can lead to performance regression relative to 22.2
@blathers-crl blathers-crl bot requested review from a team as code owners May 15, 2023 17:26
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-23.1-103106 branch from a37130d to 47cae4c Compare May 15, 2023 17:26
@blathers-crl blathers-crl bot requested a review from cucaroach May 15, 2023 17:26
@blathers-crl blathers-crl bot added blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot. labels May 15, 2023
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-23.1-103106 branch from f25697b to a3c462f Compare May 15, 2023 17:26
@blathers-crl
Copy link
Author

blathers-crl bot commented May 15, 2023

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@fqazi
Copy link
Collaborator

fqazi commented May 16, 2023

@rafiss This is expected. That query ends up trying to do a GET for each builtin into the descriptors table, so it's a different in builtin counts (there's a delta of 2 between master and 23.1).

Previously, the pg_description table was missing a virtual
index on objoid, with the addition of descriptions from all
builtins this table has much more data by default. As a result
any query joining with this table is slow. To address this,
this patch will first add a virtual index on the index
description table, and update existing incomplete indexes
on OID's to skip over builtins that refer to builtin functions.

Release note (performance improvement): improve performance
when joining with the pg_description table
@rafiss rafiss force-pushed the blathers/backport-release-23.1-103106 branch from a3c462f to dc93f55 Compare May 16, 2023 15:26
@rafiss rafiss merged commit e07095e into release-23.1 May 16, 2023
@rafiss rafiss deleted the blathers/backport-release-23.1-103106 branch May 16, 2023 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants