catalog: spin off and adopt back-reference validation level#86420
Merged
craig[bot] merged 2 commits intocockroachdb:masterfrom Sep 8, 2022
Merged
catalog: spin off and adopt back-reference validation level#86420craig[bot] merged 2 commits intocockroachdb:masterfrom
craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
Member
|
This change is |
304f6b5 to
bbd9419
Compare
Author
|
The descriptors collection is way too self-referential for this to work reliably. It calls itself for validation and hydration in ways which are impossible to reason about. I think the way to go here is to tighten the self-referential parts into an inner core object of sorts:
|
090066c to
dc8c433
Compare
Author
|
This is ready for review now, it's correct enough. |
bf297fc to
b399401
Compare
b399401 to
61bcb03
Compare
ajwerner
approved these changes
Sep 7, 2022
Contributor
ajwerner
left a comment
There was a problem hiding this comment.
Reviewed 28 of 28 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @postamar)
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go line 397 at r2 (raw file):
if _, _, inboundFKElem := scpb.FindForeignKeyConstraint(b.BackReferences(tableID)); inboundFKElem != nil { panic(scerrors.NotImplementedErrorf(t.n, fmt.Sprintf(`foreign keys to table with TTL %q are not permitted`, ns.Name)))
why fmt.Sprintf? It's already a formatting method, no? This is going to lead to redaction in a bad way
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_primary_key.go line 400 at r2 (raw file):
} if _, _, outboundFKElem := scpb.FindForeignKeyConstraint(b.QueryByID(tableID)); outboundFKElem != nil { panic(scerrors.NotImplementedErrorf(t.n,
why fmt.Sprintf? It's already a formatting method, no? This is going to lead to redaction in a bad way
Author
|
Thanks for the review! No good reason why |
added 2 commits
September 7, 2022 16:40
Previously, forward- and backward-reference-checking was done in one same validation phase, identified by the catalog.ValidationLevelCrossReferences constant and a similarly named method on catalog.Descriptor. This validation phase was exercised both when reading and when writing descriptors. The problem with this was that a corrupt back-reference would make a table unusable until descriptor surgery was performed. While this might be warranted in the context of schema changes, that's not the case when it comes to queries, which don't really care about these back-references. For this reason, we want to bypass back-reference validation when reading descriptors for the purpose of serving queries. These reads are characterized by the flag AvoidLeased being unset. To do this, this commit splits the cross-reference validation level into two. The back-reference level now exists to check the integrity of back-references directly, as well as to check that forward references also have matching back-references in the referenced descriptors. Fixes cockroachdb#85263. Release justification: low-risk, high benefit change Release note: None
Previously, it swallowed all kinds of errors, making debugging painful. Release justification: test-only change Release note: None
61bcb03 to
1a66a9b
Compare
Author
|
bors r+ |
Contributor
|
Build failed (retrying...): |
Contributor
|
Build failed (retrying...): |
Contributor
|
Build succeeded: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, forward- and backward-reference-checking was done in one
same validation phase, identified by the
catalog.ValidationLevelCrossReferences constant and a similarly named
method on catalog.Descriptor. This validation phase was exercised both
when reading and when writing descriptors.
The problem with this was that a corrupt back-reference would make
a table unusable until descriptor surgery was performed. While this
might be warranted in the context of schema changes, that's not the case
when it comes to queries, which don't really care about these
back-references. For this reason, we want to bypass back-reference
validation when reading descriptors for the purpose of serving queries.
These reads are characterized by the flag AvoidLeased being unset.
To do this, this commit splits the cross-reference validation level into
two. The back-reference level now exists to check the integrity of
back-references directly, as well as to check that forward references
also have matching back-references in the referenced descriptors.
Fixes #85263.
Release justification: low-risk, high benefit change
Release note: None
This PR is based on #87067 which is the product of what made sense for me to take action on right now regarding #86420 (comment) .