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

Scylla rejects deletions with empty-tuple restrictions #9311

Closed
dekimir opened this issue Sep 8, 2021 · 18 comments
Closed

Scylla rejects deletions with empty-tuple restrictions #9311

dekimir opened this issue Sep 8, 2021 · 18 comments

Comments

@dekimir
Copy link
Contributor

dekimir commented Sep 8, 2021

This is an unintended consequence of 1fdaeca.

Scylla:

cqlsh:ks> CREATE TABLE test (k1 int, k2 int, v int, PRIMARY KEY (k1, k2));
cqlsh:ks> DELETE FROM test WHERE k1 IN ();
InvalidRequest: Error from server: code=2200 [Invalid query] message="Invalid partition key in a modification statement"

Cassandra:

cqlsh:ks> CREATE TABLE test (k1 int, k2 int, v int, PRIMARY KEY (k1, k2));
cqlsh:ks> DELETE FROM test WHERE k1 IN ();
dekimir pushed a commit to dekimir/scylla that referenced this issue Sep 8, 2021
The empty-range check causes more bugs than it fixes.  Replace it with
an explicit check for =NULL (see scylladb#7852).

Fixes scylladb#9311.
Fixes scylladb#9290.

Tests: unit (dev), cql-pytest on Cassandra 4.0

Signed-off-by: Dejan Mircevski <dejan@scylladb.com>
@bhalevy
Copy link
Member

bhalevy commented Dec 6, 2021

@scylladb/scylla-maint we hit this issue in https://jenkins.scylladb.com/job/scylla-4.4/job/dtest-release/121/testReport/cql_additional_tests/TestCQL/empty_in_test/

Since branch-4.4 now includes 146f7b5

Please backport

@psarna
Copy link
Contributor

psarna commented Dec 6, 2021

will do

@psarna
Copy link
Contributor

psarna commented Dec 6, 2021

update: very non-trivial backport, it will take me a while, since it's no longer possible to ask the author to try and do so

@bhalevy
Copy link
Member

bhalevy commented Dec 6, 2021

@psarna Then maybe we should revert 146f7b5

@psarna
Copy link
Contributor

psarna commented Dec 6, 2021

we could, but are empty tuple restrictions ever seen outside of our test environment? Perhaps it's better to keep the patch that rejects updates with null key values at the price of not allowing empty tuple restrictions. Unless some 4.4 users actually rely on empty tuple restrictions, in which case it's better to revert

@nyh
Copy link
Contributor

nyh commented Dec 6, 2021

@psarna so you're suggesting that for 4.4, we need to disable the test, and leave the code as it is now?

@bhalevy
Copy link
Member

bhalevy commented Dec 7, 2021

On Tue, 2021-12-07 at 18:29 +0700, @aleksbykov wrote:

It is new behavior according this fix:
146f7b5

On Tue, Dec 7, 2021 at 6:10 PM Alex Bykov alex.bykov@scylladb.com wrote:

Hi all,
All rolling-upgrade tests for 4.4.8 are failed due to next error:
Cassandra.InvalidRequest: Error from server: code=2200 [Invalid query] message="Invalid partition key in a modification statement"
for next queries:

"DELETE FROM empty_in_test1 WHERE k1 IN ()",
"UPDATE empty_in_test1 SET v = 3 WHERE k1 IN () AND k2 = 2",
"DELETE FROM empty_in_test2 WHERE k1 IN ()",
"UPDATE empty_in_test2 SET v = 3 WHERE k1 IN () AND k2 = 2",

This error happened after the first node was upgraded to 4.4.8.
Was any changes validating cql queries and it is expected?
or it could be regression?

--
Thanks, Aleksandr Bykov

@psarna
Copy link
Contributor

psarna commented Dec 7, 2021

@nyh yes, that's one of the ways to deal with the problem, but it depends on at least two important factors:

  1. Are any of our users actually hit by this problem? If there are any, we should consider either a backport or a revert
  2. How long until 4.4 reaches end-of-life? Doing a complicated backport for the sake of fixing a release for 2 weeks is probably not worth it

@avikivity
Copy link
Member

It's not an important bug.

How did it start happening? Was a new test added?

@aleksbykov
Copy link
Contributor

Now, rolling upgrade stay the same. Just for v4.4.8 upgrade starts failing once first node was upgrade and these query failed with error instead of return empty results, Also i think the upgrade tests from 4.4 -> 4.5 could fail before upgrade because same query executed before upgrade

avikivity added a commit that referenced this issue Dec 8, 2021
This reverts commit 146f7b5. It
causes a regression, and needs an additional fix. The bug is not
important enough to merit this complication.

Ref #9311.
@avikivity
Copy link
Member

I reverted it from 4.4, we can revisit it but this gets us back to status quo ante.

@bhalevy
Copy link
Member

bhalevy commented Dec 14, 2021

@psarna how about 4.5?
It has 44c784c
but not 6afdc60

@bhalevy
Copy link
Member

bhalevy commented Dec 20, 2021

how about 4.5?
It has 44c784c
but not 6afdc60

@psarna ^^ (wrong user tagged previously, sorry)

@bhalevy
Copy link
Member

bhalevy commented Dec 21, 2021

avikivity added a commit that referenced this issue Dec 23, 2021
This reverts commit 44c784c. It
causes a regression without 6afdc60,
and it is too complicated to backport at this time.

Ref #9311.
@avikivity
Copy link
Member

Reverted from 4.5.

@fgelcer
Copy link

fgelcer commented Dec 23, 2021

Reverted from 4.5.

EDIT:
this job ran before the 44c784c was reverted from branch 4.5

now all rolling upgrade jobs from 4.4 and 4.5 to 4.5.3 are failing with this error:

DELETE FROM empty_in_test1 WHERE k1 IN () < t:2021-12-22 04:26:51,359 f:fill_db_data.py l:3220 c:sdcm.fill_db_data    p:ERROR > DELETE FROM empty_in_test1 WHERE k1 IN ()
Traceback (most recent call last):
  File "/jenkins/slave/workspace/scylla-4.5/rolling-upgrade/rolling-upgrade-centos7-test@2/scylla-cluster-tests/sdcm/fill_db_data.py", line 3217, in _run_db_queries
    res = session.execute(item['queries'][i])
  File "/jenkins/slave/workspace/scylla-4.5/rolling-upgrade/rolling-upgrade-centos7-test@2/scylla-cluster-tests/sdcm/utils/common.py", line 1263, in execute_verbose
    return execute_orig(*args, **kwargs)
  File "cassandra/cluster.py", line 2611, in cassandra.cluster.Session.execute
  File "cassandra/cluster.py", line 4829, in cassandra.cluster.ResponseFuture.result
cassandra.InvalidRequest: Error from server: code=2200 [Invalid query] message="Invalid partition key in a modification statement"

it means that, once these changes were reverted for 4.5 (and 4.4) the tests in SCT also had to be changed to not fail with this empty tuple...

is it still supported for 4.6?

db cluster logs
sct logs

@avikivity
Copy link
Member

The bug should be fixed in 4.6 (and not in 4.4 and 4.5, after the reverts).

@avikivity
Copy link
Member

Already backported to all vulnerable branches, removing "Backport candidate" label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants