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: parsing of negative constraints referencing attributes is broken #39801

Closed
andreimatei opened this issue Aug 21, 2019 · 4 comments · Fixed by #39824
Closed

sql: parsing of negative constraints referencing attributes is broken #39801

andreimatei opened this issue Aug 21, 2019 · 4 comments · Fixed by #39824
Assignees
Labels
A-partitioning C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@andreimatei
Copy link
Contributor

root@:26257/defaultdb> alter table t configure zone using constraints='["-x1"]';
pq: constraint "-x1" matches no existing nodes within the cluster - did you enter it correctly?

Yes computer, I've entered it correctly.

@andreimatei andreimatei added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Aug 21, 2019
@rohany
Copy link
Contributor

rohany commented Aug 21, 2019

This makes sense, we shouldn't check that the current node has the constraint on a negative constraint input. I don't think this is a result of changes I made, but I'll fix this since I've been in that part of the code recently.

@rohany
Copy link
Contributor

rohany commented Aug 22, 2019

Does this not seem like intended behavior? This checks all the nodes in the cluster to see if at least 1 has this attribute. There should be an error if no nodes have the attribute you are forbidding, because that most likely means you mistyped an attribute that the nodes actually have.

@andreimatei
Copy link
Contributor Author

I don't understand what's going on, but something's up.

./cockroach start --insecure --logtostderr --attrs=ssd:x1:x2:x3

root@:26257/defaultdb> alter table t configure zone using constraints='["-x5"]';
CONFIGURE ZONE 1

Time: 11.377ms

root@:26257/defaultdb> alter table t configure zone using constraints='["-x1"]';
pq: constraint "-x1" matches no existing nodes within the cluster - did you enter it correctly?

@rohany
Copy link
Contributor

rohany commented Aug 22, 2019

Ok, I found the cause of this -- the logic for validating these configs had a subtle flaw, and the tests for this behavior were passing due to luck i think. I'll put a fix out for this soon.

rohany added a commit to rohany/cockroach that referenced this issue Aug 22, 2019
craig bot pushed a commit that referenced this issue Aug 29, 2019
39824: sql: Fix bug with negative parsing validation for zone constraints r=solongordon a=rohany

Fixes #39801.

This PR fixes the parsing issues brought up in #39801, and refactors the test cases for zone validation to specifically test this behavior. Previously this behavior was tested, but falsely passed due to a logic error.

Release note: None

Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>
@craig craig bot closed this as completed in a3257c9 Aug 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-partitioning C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants