From a4a318f39454f5af6ebf2371fca9560f624802cd Mon Sep 17 00:00:00 2001 From: Nadav Har'El Date: Mon, 6 Mar 2023 00:08:52 +0200 Subject: [PATCH] cql: USING TTL 0 means unlimited, not default TTL Our documentation states that writing an item with "USING TTL 0" means it should never expire. This should be true even if the table has a default TTL. But Scylla mistakenly handled "USING TTL 0" exactly like having no USING TTL at all (i.e., it took the default TTL, instead of unlimited). We had two xfailing tests demonstrating that Scylla's behavior in this is different from Cassandra. Scylla's behavior in this case was also undocumented. By the way, Cassandra used to have the same bug (CASSANDRA-11207) but it was fixed already in 2016 (Cassandra 3.6). So in this patch we fix Scylla's "USING TTL 0" behavior to match the documentation and Cassandra's behavior since 2016. One xfailing test starts to pass and the second test passes this bug and fails on a different one. This patch also adds a third test for "USING TTL ?" with UNSET_VALUE - it behaves, on both Scylla and Cassandra, like a missing "USING TTL". The origin of this bug was that after parsing the statement, we saved the USING TTL in an integer, and used 0 for the case of no USING TTL given. This meant that we couldn't tell if we have USING TTL 0 or no USING TTL at all. This patch uses an std::optional so we can tell the case of a missing USING TTL from the case of USING TTL 0. Fixes #6447 Signed-off-by: Nadav Har'El Closes #13079 --- cql3/attributes.cc | 5 +++-- cql3/attributes.hh | 2 +- cql3/statements/modification_statement.cc | 6 ++++-- cql3/statements/modification_statement.hh | 2 +- cql3/update_parameters.hh | 18 +++++------------- .../validation/operations/insert_test.py | 2 +- test/cql-pytest/test_ttl.py | 18 +++++++++++++++++- test/cql-pytest/test_unset.py | 4 ++++ 8 files changed, 36 insertions(+), 21 deletions(-) diff --git a/cql3/attributes.cc b/cql3/attributes.cc index f2ea964a73..08cf7957c0 100644 --- a/cql3/attributes.cc +++ b/cql3/attributes.cc @@ -10,6 +10,7 @@ #include "cql3/attributes.hh" #include "cql3/column_identifier.hh" +#include namespace cql3 { @@ -55,9 +56,9 @@ int64_t attributes::get_timestamp(int64_t now, const query_options& options) { } } -int32_t attributes::get_time_to_live(const query_options& options) { +std::optional attributes::get_time_to_live(const query_options& options) { if (!_time_to_live.has_value() || _time_to_live_unset_guard.is_unset(options)) - return 0; + return std::nullopt; cql3::raw_value tval = expr::evaluate(*_time_to_live, options); if (tval.is_null()) { diff --git a/cql3/attributes.hh b/cql3/attributes.hh index dce6146afc..53280fbe87 100644 --- a/cql3/attributes.hh +++ b/cql3/attributes.hh @@ -45,7 +45,7 @@ public: int64_t get_timestamp(int64_t now, const query_options& options); - int32_t get_time_to_live(const query_options& options); + std::optional get_time_to_live(const query_options& options); db::timeout_clock::duration get_timeout(const query_options& options) const; diff --git a/cql3/statements/modification_statement.cc b/cql3/statements/modification_statement.cc index 50aa43685c..60dfea4794 100644 --- a/cql3/statements/modification_statement.cc +++ b/cql3/statements/modification_statement.cc @@ -17,6 +17,7 @@ #include "cql3/util.hh" #include "validation.hh" #include "db/consistency_level_validations.hh" +#include #include #include #include @@ -95,8 +96,9 @@ bool modification_statement::is_timestamp_set() const { return attrs->is_timestamp_set(); } -gc_clock::duration modification_statement::get_time_to_live(const query_options& options) const { - return gc_clock::duration(attrs->get_time_to_live(options)); +std::optional modification_statement::get_time_to_live(const query_options& options) const { + std::optional ttl = attrs->get_time_to_live(options); + return ttl ? std::make_optional(*ttl) : std::nullopt; } future<> modification_statement::check_access(query_processor& qp, const service::client_state& state) const { diff --git a/cql3/statements/modification_statement.hh b/cql3/statements/modification_statement.hh index 79a6855306..83be9b411e 100644 --- a/cql3/statements/modification_statement.hh +++ b/cql3/statements/modification_statement.hh @@ -127,7 +127,7 @@ public: bool is_timestamp_set() const; - gc_clock::duration get_time_to_live(const query_options& options) const; + std::optional get_time_to_live(const query_options& options) const; virtual future<> check_access(query_processor& qp, const service::client_state& state) const override; diff --git a/cql3/update_parameters.hh b/cql3/update_parameters.hh index 43e3b8cc4f..ea3e1e526f 100644 --- a/cql3/update_parameters.hh +++ b/cql3/update_parameters.hh @@ -86,7 +86,7 @@ public: }; // Note: value (mutation) only required to contain the rows we are interested in private: - const gc_clock::duration _ttl; + const std::optional _ttl; // For operations that require a read-before-write, stores prefetched cell values. // For CAS statements, stores values of conditioned columns. // Is a reference to an outside prefetch_data container since a CAS BATCH statement @@ -99,7 +99,7 @@ public: const query_options& _options; update_parameters(const schema_ptr schema_, const query_options& options, - api::timestamp_type timestamp, gc_clock::duration ttl, const prefetch_data& prefetched) + api::timestamp_type timestamp, std::optional ttl, const prefetch_data& prefetched) : _ttl(ttl) , _prefetched(prefetched) , _timestamp(timestamp) @@ -120,11 +120,7 @@ public: } atomic_cell make_cell(const abstract_type& type, const raw_value_view& value, atomic_cell::collection_member cm = atomic_cell::collection_member::no) const { - auto ttl = _ttl; - - if (ttl.count() <= 0) { - ttl = _schema->default_time_to_live(); - } + auto ttl = this->ttl(); return value.with_value([&] (const FragmentedView auto& v) { if (ttl.count() > 0) { @@ -136,11 +132,7 @@ public: }; atomic_cell make_cell(const abstract_type& type, const managed_bytes_view& value, atomic_cell::collection_member cm = atomic_cell::collection_member::no) const { - auto ttl = _ttl; - - if (ttl.count() <= 0) { - ttl = _schema->default_time_to_live(); - } + auto ttl = this->ttl(); if (ttl.count() > 0) { return atomic_cell::make_live(type, _timestamp, value, _local_deletion_time + ttl, ttl, cm); @@ -162,7 +154,7 @@ public: } gc_clock::duration ttl() const { - return _ttl.count() > 0 ? _ttl : _schema->default_time_to_live(); + return _ttl.value_or(_schema->default_time_to_live()); } gc_clock::time_point expiry() const { diff --git a/test/cql-pytest/cassandra_tests/validation/operations/insert_test.py b/test/cql-pytest/cassandra_tests/validation/operations/insert_test.py index 8fe59f7c52..0270524be8 100644 --- a/test/cql-pytest/cassandra_tests/validation/operations/insert_test.py +++ b/test/cql-pytest/cassandra_tests/validation/operations/insert_test.py @@ -161,7 +161,7 @@ def testInsertWithAStaticColumn(cql, test_keyspace, forceFlush): "INSERT INTO %s (partitionKey, clustering_2, staticValue) VALUES (0, 0, 'A')") # Reproduces #6447 and #12243: -@pytest.mark.xfail(reason="Issue #6447, #12243") +@pytest.mark.xfail(reason="Issue #12243") def testInsertWithDefaultTtl(cql, test_keyspace): secondsPerMinute = 60 with create_table(cql, test_keyspace, f"(a int PRIMARY KEY, b int) WITH default_time_to_live = {10*secondsPerMinute}") as table: diff --git a/test/cql-pytest/test_ttl.py b/test/cql-pytest/test_ttl.py index 631a83fe9b..593122f13f 100644 --- a/test/cql-pytest/test_ttl.py +++ b/test/cql-pytest/test_ttl.py @@ -7,6 +7,7 @@ ############################################################################# from util import new_test_table, unique_key_int +from cassandra.query import UNSET_VALUE import pytest import time @@ -45,10 +46,25 @@ def test_basic_default_ttl(cql, table_ttl_1): # ttl set for the table. Here we check that also in the special case of # "using ttl 0" (which means the item should never expire) it also overrides # the default TTL. Reproduces issue #9842. -@pytest.mark.xfail(reason="Issue #9842") def test_default_ttl_0_override(cql, table_ttl_100): p = unique_key_int() cql.execute(f'INSERT INTO {table_ttl_100} (p, v) VALUES ({p}, 1) USING TTL 0') # We can immediately check that this item's TTL is "null", meaning it # will never expire. There's no need to have any sleeps. assert list(cql.execute(f'SELECT ttl(v) from {table_ttl_100} where p={p}')) == [(None,)] + +# Whereas a zero TTL overrides the default TTL, an unset TTL doesn't - and +# leaves the default TTL. This means that the text in Cassandra's NEWS.txt +# which claims that "an unset bind ttl is treated as 'unlimited'" is not +# accurate - it's only unlimited if there is no default TTL for the table, +# and if there is, this default TTL is used instead of unlimited. In other +# words, an UNSET_VALUE TTL behaves exactly like not specifying a TTL at all. +# See also test_unset.py::test_unset_ttl() +def test_default_ttl_unset_override(cql, table_ttl_100): + p = unique_key_int() + stmt = cql.prepare(f'INSERT INTO {table_ttl_100} (p, v) VALUES ({p}, 1) USING TTL ?') + cql.execute(stmt, [UNSET_VALUE]) + # At this point, reading ttl(v) would normally return 99, but it's + # possible in case of delays to be somewhat lower. But it wouldn't + # be None - which would mean that the item will never expire. + assert list(cql.execute(f'SELECT ttl(v) from {table_ttl_100} where p={p}')) != [(None,)] diff --git a/test/cql-pytest/test_unset.py b/test/cql-pytest/test_unset.py index aed8a702dd..4d81cabece 100644 --- a/test/cql-pytest/test_unset.py +++ b/test/cql-pytest/test_unset.py @@ -115,6 +115,10 @@ def test_unset_list_append(cql, table1, cassandra_bug): # According to Cassandra's NEWS.txt, "an unset bind ttl is treated as # 'unlimited'". It shouldn't skip the write. +# Note that the NEWS.txt is not accurate: An unset ttl isn't really treated +# as unlimited, but rather as the default ttl set on the table. The default +# ttl is usually unlimited, but not always. We test that case in +# test_ttl.py::test_default_ttl_unset() def test_unset_ttl(cql, table1): p = unique_key_int() # First write using a normal TTL: