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

sql: setting zone configs for non-materialized views should be forbidden #57478

Closed
postamar opened this issue Dec 3, 2020 · 4 comments · Fixed by #60592
Closed

sql: setting zone configs for non-materialized views should be forbidden #57478

postamar opened this issue Dec 3, 2020 · 4 comments · Fixed by #60592
Labels
A-zone-configs C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. E-easy Easy issue to tackle, requires little or no CockroachDB experience good first issue

Comments

@postamar
Copy link
Contributor

postamar commented Dec 3, 2020

isn't it a bug that we even allow setting zone configs on (non-materialized) views in the first place? Is there any point to this?

Originally posted by @lucy-zhang in #57147 (comment)

Description: It's currently possible to run ALTER TABLE ... CONFIGURE ZONE statements on views. Considering that zone configs are meant to describe the physical layout of the data in a table, it doesn't make sense to me either that executing such a statement be possible on a non-physical table.

To reproduce:

demo@127.0.0.1:26257/movr> CREATE VIEW v AS SELECT 1; ALTER TABLE v CONFIGURE ZONE USING gc.ttlseconds = 100000;
CONFIGURE ZONE 1

I would instead expect an error like this:

ERROR: "v" is not a table
SQLSTATE: 42809

Looking at https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/set_zone_config.go nothing is there to indicate that this behaviour is allowed for a good reason so I'm tentatively filing this as a bug despite having only a superficial understanding of things. If it turns out that it's not a bug then perhaps adding a comment to that effect in that source file might be helpful.

@postamar postamar added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-zone-configs labels Dec 3, 2020
@postamar postamar changed the title sql: setting zone configs for views should be forbidden sql: setting zone configs for non-materialized views should be forbidden Dec 3, 2020
@rafiss rafiss added E-easy Easy issue to tackle, requires little or no CockroachDB experience good first issue labels Jan 12, 2021
@naivcit
Copy link
Contributor

naivcit commented Jan 14, 2021

Hi @rafiss can I take this one ?

@rafiss
Copy link
Collaborator

rafiss commented Jan 14, 2021

@naivcit Yes, thank you! We would appreciate your contribution.

to get started, look at pkg/sql/set_zone_config.go In the startExec function, after fetching the table descriptor with resolveTableForZone, you will likely need to check if table.IsPhysicalTable() and return an error message if it is not physical.

@jyz0309
Copy link

jyz0309 commented Jan 21, 2021

hi,@naivcit have you already started working on this? May I take it?

@naivcit
Copy link
Contributor

naivcit commented Jan 21, 2021

Hey @jyz0309 yeah I am already working on this , sorry, will post a PR soon .

naivcit added a commit to naivcit/cockroach that referenced this issue Jan 21, 2021
Fixes cockroachdb#57478.

Release note (sql change): Setting of zone configs for non physical tables is
now forbidden .
naivcit added a commit to naivcit/cockroach that referenced this issue Feb 15, 2021
Release note (sql change): Setting of zone configs for non physical tables is
now forbidden .
naivcit added a commit to naivcit/cockroach that referenced this issue Feb 16, 2021
Fixes cockroachdb#57478.

Release note (sql change): Setting of zone configs for non physical tables is
now forbidden .
craig bot pushed a commit that referenced this issue Feb 16, 2021
60592: sql: Forbid setting  of zone config for non physical tables r=otan a=naivcit

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

Co-authored-by: Ratnesh Mishra <ratnesrace@gmail.com>
@craig craig bot closed this as completed in 49efdb5 Feb 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-zone-configs C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. E-easy Easy issue to tackle, requires little or no CockroachDB experience good first issue
Projects
None yet
4 participants