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

sql: Forbid setting of zone config for non physical tables #60592

Merged
merged 1 commit into from
Feb 16, 2021
Merged

sql: Forbid setting of zone config for non physical tables #60592

merged 1 commit into from
Feb 16, 2021

Conversation

naivcit
Copy link
Contributor

@naivcit naivcit commented Feb 15, 2021 •
edited
Loading

Fixes #57478
Release note (sql change): Setting of zone configs for non physical tables is
now forbidden .

@blathers-crl
Copy link

blathers-crl bot commented Feb 15, 2021

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

I was unable to automatically find a reviewer. You can try CCing one of the following members:

  • A person you worked with closely on this PR.
  • The person who created the ticket, or a CRDB organization member involved with the ticket (author, commenter, etc.).
  • Join our community slack channel and ask on #contributors.
  • Try find someone else from here.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl blathers-crl bot added O-community Originated from the community X-blathers-untriaged blathers was unable to find an owner labels Feb 15, 2021
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@naivcit naivcit changed the title Fixes #57478. sql: Forbid setting of zone config for non physical tables Feb 15, 2021
@naivcit
Copy link
Contributor Author

naivcit commented Feb 15, 2021 •
edited
Loading

cc @rafiss opened a new PR for #57478 as there were a lot of merge conflicts with the older one , please have a look :)

otan
otan reviewed Feb 16, 2021
View reviewed changes
Copy link
Contributor

@otan otan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I have some improvements to the rror messaging.

pkg/sql/set_zone_config.go Outdated Show resolved Hide resolved
pkg/sql/set_zone_config.go Outdated Show resolved Hide resolved
@blathers-crl blathers-crl bot requested a review from otan February 16, 2021 03:49
@blathers-crl
Copy link

blathers-crl bot commented Feb 16, 2021

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

otan
otan reviewed Feb 16, 2021
View reviewed changes
pkg/sql/set_zone_config.go Outdated
@@ -342,6 +342,12 @@ func (n *setZoneConfigNode) startExec(params runParams) error {
return err
}

// If the table descriptor is resolved but is not a
// physical table then return an error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit: this needs a full stop (.) at the end!

@blathers-crl blathers-crl bot requested a review from otan February 16, 2021 03:54
@otan otan force-pushed the rats/zoe-config-validate branch 2 times, most recently from 317a001 to 7df513c Compare February 16, 2021 04:11
otan
otan approved these changes Feb 16, 2021
View reviewed changes
Copy link
Contributor

@otan otan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

bors r+

@craig
Copy link
Contributor

craig bot commented Feb 16, 2021

Build failed:

@naivcit @otan
sql: Forbid setting of zone config for non physical tables
49efdb5 
Fixes cockroachdb#57478.

Release note (sql change): Setting of zone configs for non physical tables is
now forbidden .
@naivcit
Copy link
Contributor Author

naivcit commented Feb 16, 2021

The test TestDropPhysicalTableGC involves setting up zone config for a view and since we are restricting that here so I think the test case is failing . What should we do here ? cc @otan

@otan
Copy link
Contributor

otan commented Feb 16, 2021

i've pushed a fix. we should not apply the zone config the test attempts to set up if it's a view!

@otan
Copy link
Contributor

otan commented Feb 16, 2021

bors r+

@craig
Copy link
Contributor

craig bot commented Feb 16, 2021

Build succeeded:

@craig craig bot merged commit 68b86d5 into cockroachdb:master Feb 16, 2021
@irfansharif irfansharif mentioned this pull request Mar 15, 2021
Closed
@jseldess jseldess mentioned this pull request Mar 23, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@otan otan otan approved these changes

Assignees
No one assigned
Labels
O-community Originated from the community X-blathers-untriaged blathers was unable to find an owner
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

sql: setting zone configs for non-materialized views should be forbidden
3 participants
@naivcit @cockroach-teamcity @otan