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

catalog/tabledesc: improve view validation in ValidateCrossReferences #63147

Closed
ajwerner opened this issue Apr 6, 2021 · 2 comments · Fixed by #70632
Closed

catalog/tabledesc: improve view validation in ValidateCrossReferences #63147

ajwerner opened this issue Apr 6, 2021 · 2 comments · Fixed by #70632
Assignees
@postamar
Labels
A-schema-descriptors Relating to SQL table/db descriptor handling. 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)

Comments

@ajwerner
Copy link
Contributor

ajwerner commented Apr 6, 2021 •
edited by cockroach-jira-scripts
Loading

Is your feature request related to a problem? Please describe.

We don't currently validate the cross references. This is a bummer. We won't want to enforce validation of them on read because of existing bugs but we would like to prevent new errors from being created.

See

cockroach/pkg/sql/catalog/tabledesc/validate_test.go

Lines 93 to 98 in bd62e2c

"DependsOn": {
status: todoIAmKnowinglyAddingTechDebt,
reason: "initial import: TODO(features): add validation"},
"DependedOnBy": {
status: todoIAmKnowinglyAddingTechDebt,
reason: "initial import: TODO(features): add validation"},

Describe the solution you'd like

We should validate the cross references.

Additional context

Relates to #63065

Epic: CRDB-1519

Jira issue: CRDB-6446
@ajwerner ajwerner added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-schema-descriptors Relating to SQL table/db descriptor handling. labels Apr 6, 2021
@ajwerner ajwerner mentioned this issue Apr 6, 2021
Closed
@jlinder jlinder added the T-sql-schema-deprecated Use T-sql-foundations instead label Jun 16, 2021
@ajwerner
Copy link
Contributor Author

ajwerner commented Aug 3, 2021

These are for views.

@ajwerner ajwerner changed the title catalog/tabledesc: validate DependsOn and DependedOnBy in ValidateCrossReferences catalog/tabledesc: improve view validation in ValidateCrossReferences Aug 3, 2021
@ajwerner
Copy link
Contributor Author

ajwerner commented Aug 3, 2021

In In #60775 we added code to retrieve the descriptors from these two fields but we don't do any work to check the backreferences in those descriptors. We should.

Other validation missing on views:

  • Privileges

@postamar postamar self-assigned this Aug 3, 2021
@postamar postamar mentioned this issue Sep 23, 2021
Merged
postamar pushed a commit to postamar/cockroach that referenced this issue Sep 23, 2021
tabledesc: improve view validation
7de5ac7 
Previously, we did not validate the DependsOn and DependedOnBy
cross-references. This commit adds this validation and also fixes a bug
in which privileges were not validated for non-physical tables.

Fixes cockroachdb#63147.

Release note: None
postamar pushed a commit to postamar/cockroach that referenced this issue Sep 24, 2021
tabledesc: improve view validation
1e113f4 
Previously, we did not validate the DependsOn and DependedOnBy
cross-references. This commit adds this validation and also fixes a bug
in which privileges were not validated for non-physical tables.

Fixes cockroachdb#63147.

Release note: None
postamar pushed a commit to postamar/cockroach that referenced this issue Nov 8, 2021
tabledesc: improve view validation
7359fa7 
Previously, we did not validate the DependsOn and DependedOnBy
cross-references. This commit adds this validation and also fixes a few
bugs:
- privileges were not validated for non-physical tables.
- sequence dependencies were improperly updated by the schema changer.

Fixes cockroachdb#63147.

Release note: None
postamar pushed a commit to postamar/cockroach that referenced this issue Nov 11, 2021
tabledesc: improve view validation
9f2d272 
Previously, we did not validate the DependsOn and DependedOnBy
cross-references. This commit adds this validation and also fixes a few
bugs:
- privileges were not validated for non-physical tables.
- sequence dependencies were improperly updated by the schema changer.

Fixes cockroachdb#63147.

Release note: None
postamar pushed a commit to postamar/cockroach that referenced this issue Jan 26, 2022
tabledesc: improve view validation
feade18 
Previously, we did not validate the DependsOn and DependedOnBy
cross-references. This commit adds this validation and also fixes a few
bugs:
- privileges were not validated for non-physical tables.
- sequence dependencies were improperly updated by the schema changer.

Fixes cockroachdb#63147.

Release note: None
postamar pushed a commit to postamar/cockroach that referenced this issue Jan 27, 2022
tabledesc: improve view validation
be17ab9 
Previously, we did not validate the DependsOn, DependsOnTyoe and
DependedOnBy cross-references. This commit adds this validation and
also fixes a few bugs:
- privileges were not validated for non-physical tables.
- sequence dependencies were improperly updated by the schema changer.

Unfortunately, there remain a number of unfortunate bugs around the
inconsistent handling of these fields:
- back-references are not systematically present.
- ColumnIDs in DependedOnBy has a different meaning depending on whether
  the table is a sequence or not.

These inconsistencies need to be ironed out by a cluster migration. This
in turn will require changes to the PostRestoreChanges descriptor
builder method. It probably also requires version-gating validation
checks.

Fixes cockroachdb#63147.

Release note: None
craig bot pushed a commit that referenced this issue Feb 7, 2022
Merge #70632
fc1957a 
70632: tabledesc: improve view validation r=postamar a=postamar

Previously, we did not validate the DependsOn and DependedOnBy
cross-references. This commit adds this validation and also fixes a bug
in which privileges were not validated for non-physical tables.

Fixes #63147.

Release note: None

Co-authored-by: Marius Posta <[email protected]>
@craig craig bot closed this as completed in 718222f Feb 7, 2022
@exalate-issue-sync exalate-issue-sync bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed T-sql-schema-deprecated Use T-sql-foundations instead labels May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@postamar postamar

Labels
A-schema-descriptors Relating to SQL table/db descriptor handling. 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)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

tabledesc: improve view validation postamar/cockroach
3 participants
@jlinder @ajwerner @postamar