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.

Sign up for GitHub

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

Jump to bottom

upgrades: Add sanity check that we only repair descriptors that show up in invalid_objects table. #110906

Closed
rimadeodhar opened this issue Sep 19, 2023 · 3 comments · Fixed by #112700
Closed

upgrades: Add sanity check that we only repair descriptors that show up in invalid_objects table. #110906

rimadeodhar opened this issue Sep 19, 2023 · 3 comments · Fixed by #112700
Assignees
@annrpom
Labels
branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) GA-blocker O-postmortem Originated from a Postmortem action item. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@rimadeodhar
Copy link
Collaborator

rimadeodhar commented Sep 19, 2023 •
edited by cockroach-jira-scripts
Loading

We should add a sanity check within the automated descriptor corruption repair that ensures we only attempt repairs for descriptors that show up in the invalid_objects table.

Jira issue: CRDB-31670
@rimadeodhar rimadeodhar added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Sep 19, 2023
@rimadeodhar rimadeodhar added the O-postmortem Originated from a Postmortem action item. label Sep 19, 2023
@rimadeodhar
Copy link
Collaborator Author

rimadeodhar commented Sep 19, 2023 •
edited
Loading

Follow up for this

@rafiss
Copy link
Collaborator

rafiss commented Oct 9, 2023

We could also consider using the repairable_catalog_corruptions table.

@rafiss
Copy link
Collaborator

rafiss commented Oct 9, 2023

I see that the repair step already has this: https://github.com/cockroachdb/cockroach/blob/be961bf36f01162a0787c6d81be2d2be1016ff58/pkg/upgrade/upgrades/first_upgrade.go#L58C1-L61C4

That check should cause the code to short-circuit for any descriptor that is not invalid. Let's confirm if that is true - if so, we could try adding a test to assert that, and close this out.

@annrpom annrpom mentioned this issue Oct 19, 2023
Merged
annrpom added a commit to annrpom/cockroach that referenced this issue Oct 19, 2023
@annrpom
upgrades: assert descriptor repair has correct set of targets
1873b7c 
This patch adds a test to assert that during automated repair of
corrupt descriptors, we short-circuit for any descriptor that is
not invalid.

Epic: none
Fixes: cockroachdb#110906

Release note: None
@rafiss rafiss added GA-blocker branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 labels Oct 20, 2023
annrpom added a commit to annrpom/cockroach that referenced this issue Oct 30, 2023
@annrpom
upgrades: assert descriptor repair has correct set of targets
62827f3 
This patch adds a test to assert that during automated repair of
corrupt descriptors, we do not try to repair a descriptor that
is not corrupted.

Epic: none
Fixes: cockroachdb#110906

Release note: None
craig bot pushed a commit that referenced this issue Nov 2, 2023
@annrpom @mgartner
Merge #112700 #113319
8e059c2 
112700: upgrades: assert descriptor repair has correct set of targets r=rafiss a=annrpom

This patch adds a test to assert that during automated repair of
corrupt descriptors, we do not try to repair a descriptor that
is not corrupted.

While testing, it was discovered that function descriptors (regardless of corruption/repair) had their descriptor version increased due to `grantExecuteToPublicOnAllFunctions` being called on each function during a cluster upgrade; however, we noticed that the functions we were testing already had execute privileges for `public`. Thus, a check was added in said logic that ensures functions in this situation (where public already has execute priv. for the func) do not try to grant execute again.

Epic: none
Fixes: #110906

Release note: None

113319: opt: optimize FuncDepSet.ReduceCols r=mgartner a=mgartner

#### opt/bench: add slow-query-5

This commit adds the new `slow-query-5` benchmark which tests a 40+ -way
join modelled off of a real-world production query. The query stresses
the efficiency of the optimizer, particularly in building and
manipulating thousands of functional dependencies for the thousands of
possible join orderings.

Release note: None

#### opt: optimize FuncDepSet.ReduceCols

`FuncDepSet.ReduceCols` is used frequently to manipulate `FuncDepSet`s.
In queries with many tables, columns, and joins, it can contribute
significantly to the latency of query planning, due to the `O(n²)` time
complexity of calculating a transitive closure (see
`FuncDepSet.inClosureOf`), where `n` is the number of dependencies in
the set, and the relatively expensive `SubsetOf` set operation performed
multiple times for each of the `n²` iterations.

This commit optimizes the `ReduceCols` by adding a fast path that can
more quickly determine if a column cannot be reduced by checking if it
exists in none of the `to` sets of the functional dependencies. This
avoids having to calculate the transitive closure in many cases,
significantly speeding up query planning time.

Epic: None

Release note (performance improvement): Query planning time has been
reduced significantly for some queries in which many tables are joined.


Co-authored-by: Annie Pompa <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
@craig craig bot closed this as completed in 64c1f6c Nov 2, 2023
@blathers-crl blathers-crl bot mentioned this issue Nov 2, 2023
Merged
blathers-crl bot pushed a commit that referenced this issue Nov 2, 2023
@annrpom
upgrades: assert descriptor repair has correct set of targets
fa174bf 
This patch adds a test to assert that during automated repair of
corrupt descriptors, we do not try to repair a descriptor that
is not corrupted.

Epic: none
Fixes: #110906

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@annrpom annrpom

Labels
branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) GA-blocker O-postmortem Originated from a Postmortem action item. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
No open projects
SQL Foundations
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

upgrades: assert descriptor repair has correct set of targets annrpom/cockroach
3 participants
@rafiss @annrpom @rimadeodhar