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

tabledesc: add missing row-level TTL validation unit test cases #86410

Closed
postamar opened this issue Aug 18, 2022 · 3 comments · Fixed by #86269
Closed

tabledesc: add missing row-level TTL validation unit test cases #86410

postamar opened this issue Aug 18, 2022 · 3 comments · Fixed by #86269
Labels
branch-master Failures and bugs on the master branch. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker

Comments

@postamar
Copy link
Contributor

postamar commented Aug 18, 2022

I looked at the validation code today and noticed that while the row-level TTL configuration in a table descriptor does indeed have validation logic, this logic is never tested anywhere. This must have slipped through when the feature was squeezed into the last release.

The expiration expression, in particular, has no tests checking that the referenced columns actually exist, and I can't find any logic to that effect.

This is a bigger deal than it sounds. Not only has this testing repeatedly surfaced bugs and regressions; but having well-tested validation logic is a key element in lowering SQL Schema's support burden.

Accordingly, I'm marking this as a GA-blocker.

Jira issue: CRDB-18746

@postamar postamar added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Aug 18, 2022
@postamar postamar added branch-master Failures and bugs on the master branch. GA-blocker labels Aug 18, 2022
@ajwerner
Copy link
Contributor

@rafiss would you say that your open PR fixes this issue, or no?

@rafiss
Copy link
Collaborator

rafiss commented Aug 19, 2022

yeah #86269 will close this

@postamar
Copy link
Contributor Author

I don't share this belief that logic tests are enough, though obviously they are very welcome. We can't anticipate how corruption can occur and I'd sleep better at night if we had some unit test cases to make sure that corruptions will get detected, particularly when it comes to bad column references in the expiration expr.

@craig craig bot closed this as completed in 0c9ac43 Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-master Failures and bugs on the master branch. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants