Skip to content

Commit

Permalink
Merge 'cql: fix crash on empty clustering range in LWT' from Jan Ciołek
Browse files Browse the repository at this point in the history
LWT queries with empty clustering range used to cause a crash.
For example in:
```cql
UPDATE tab SET r = 9000 WHERE p = 1  AND c = 2 AND c = 2000 IF r = 3
```
The range of `c` is empty - there are no valid values.

This caused a segfault when accessing the `first` range:
```c++
op.ranges.front()
```

Cassandra rejects such queries at the preparation stage. It doesn't allow two `EQ` restriction on the same clustering column when an IF is involved.
We reject them during runtime, which is a worse solution. The user can prepare a query with `c = ? AND c = ?`, and then run it, but unexpectedly it will throw an `invalid_request_exception` when the two bound variables are different.

We could ban such queries as well, we already ban the usage of `IN` in conditional statements. The problem is that this would be a breaking change.

A better solution would be to allow empty ranges in `LWT` statements. When an empty range is detected we just wouldn't apply the change. This would be a larger change, for now let's just fix the crash.

Fixes: #13129

Closes #14429

* github.com:scylladb/scylladb:
  modification_statement: reject conditional statements with empty clustering key
  statements/cas_request: fix crash on empty clustering range in LWT
  • Loading branch information
nyh committed Jun 28, 2023
2 parents 586102b + bfbc3d7 commit 49c8c06
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 1 deletion.
6 changes: 6 additions & 0 deletions cql3/statements/cas_request.cc
Expand Up @@ -123,6 +123,9 @@ std::optional<mutation> cas_request::apply(foreign_ptr<lw_shared_ptr<query::resu

cas_request::old_row cas_request::find_old_row(const cas_row_update& op) const {
static const clustering_key empty_ckey = clustering_key::make_empty();
if (_key.empty()) {
throw exceptions::invalid_request_exception("Empty partition key range");
}
const partition_key& pkey = _key.front().start()->value().key().value();
// We must ignore statement clustering column restriction when
// choosing a row to check the conditions. If there is no
Expand All @@ -134,6 +137,9 @@ cas_request::old_row cas_request::find_old_row(const cas_row_update& op) const {
// CREATE TABLE t(p int, c int, s int static, v int, PRIMARY KEY(p, c));
// INSERT INTO t(p, s) VALUES(1, 1);
// UPDATE t SET v=1 WHERE p=1 AND c=1 IF s=1;
if (op.ranges.empty()) {
throw exceptions::invalid_request_exception("Empty clustering range");
}
const clustering_key& ckey = op.ranges.front().start() ? op.ranges.front().start()->value() : empty_ckey;
auto row = _rows.find_row(pkey, ckey);
auto ckey_ptr = &ckey;
Expand Down
4 changes: 4 additions & 0 deletions cql3/statements/modification_statement.cc
Expand Up @@ -318,6 +318,10 @@ modification_statement::execute_with_condition(query_processor& qp, service::que
throw exceptions::invalid_request_exception(format("Unrestricted partition key in a conditional {}",
type.is_update() ? "update" : "deletion"));
}
if (ranges.empty()) {
throw exceptions::invalid_request_exception(format("Unrestricted clustering key in a conditional {}",
type.is_update() ? "update" : "deletion"));
}

auto request = seastar::make_shared<cas_request>(s, std::move(keys));
// cas_request can be used for batches as well single statements; Here we have just a single
Expand Down
1 change: 0 additions & 1 deletion test/cql-pytest/test_lwt.py
Expand Up @@ -73,7 +73,6 @@ def test_lwt_empty_partition_range(cql, table1):
# Generate an LWT update where there is no value for the clustering key,
# as the WHERE restricts it using `c = 2 AND c = 3`.
# Such queries are rejected.
@pytest.mark.skip(reason="crashes scylla, see issue #13129")
def test_lwt_empty_clustering_range(cql, table1):
with pytest.raises(InvalidRequest):
cql.execute(f"UPDATE {table1} SET r = 9000 WHERE p = 1 AND c = 2 AND c = 2000 IF r = 3")
Expand Down

0 comments on commit 49c8c06

Please sign in to comment.