Merge 'Validate compaction strategy options in prepare' from Aleksand…

…ra Martyniuk

Table properties validation is performed on statement execution.
Thus, when one attempts to create a table with invalid options,
an incorrect command gets committed in Raft. But then its
application fails, leading to a raft machine being stopped.

Check table properties when create and alter statements are prepared.

Fixes: #14710.

Closes #15091

* github.com:scylladb/scylladb:
  cql3: statements: delete execute override
  cql3: statements: call check_restricted_table_properties in prepare
  cql3: statements: pass data_dictionary::database to check_restricted_table_properties

cql3/statements/alter_table_statement.cc

@@ -398,20 +398,19 @@ alter_table_statement::prepare_schema_mutations(query_processor& qp, api::timest
 
 std::unique_ptr<cql3::statements::prepared_statement>
 cql3::statements::alter_table_statement::prepare(data_dictionary::database db, cql_stats& stats) {
+    auto t = db.try_find_table(keyspace(), column_family());
+    std::optional<schema_ptr> s = t ? std::make_optional(t->schema()) : std::nullopt;
+    std::optional<sstring> warning = check_restricted_table_properties(db, s, keyspace(), column_family(), *_properties);
+    if (warning) {
+        mylogger.warn("{}", *warning);
+    }
     return std::make_unique<prepared_statement>(make_shared<alter_table_statement>(*this));
 }
 
 future<::shared_ptr<messages::result_message>>
 alter_table_statement::execute(query_processor& qp, service::query_state& state, const query_options& options, std::optional<service::group0_guard> guard) const {
-    auto s = validation::validate_column_family(qp.db(), keyspace(), column_family());
-    std::optional<sstring> warning = check_restricted_table_properties(qp, s, keyspace(), column_family(), *_properties);
-    return schema_altering_statement::execute(qp, state, options, std::move(guard)).then([warning = std::move(warning)] (::shared_ptr<messages::result_message> msg) {
-        if (warning) {
-            msg->add_warning(*warning);
-            mylogger.warn("{}", *warning);
-        }
-        return msg;
-    });
+    validation::validate_column_family(qp.db(), keyspace(), column_family());
+    return schema_altering_statement::execute(qp, state, options, std::move(guard));
 }
 
 }

cql3/statements/create_table_statement.cc

@@ -181,6 +181,10 @@ std::unique_ptr<prepared_statement> create_table_statement::raw_statement::prepa
     if (_properties.properties()->get_synchronous_updates_flag()) {
         throw exceptions::invalid_request_exception(format("The synchronous_updates option is only applicable to materialized views, not to base tables"));
     }
+    std::optional<sstring> warning = check_restricted_table_properties(db, std::nullopt, keyspace(), column_family(), *_properties.properties());
+    if (warning) {
+        mylogger.warn("{}", *warning);
+    }
     const bool has_default_ttl = _properties.properties()->get_default_time_to_live() > 0;
 
     auto stmt = ::make_shared<create_table_statement>(*_cf_name, _properties.properties(), _if_not_exists, _static_columns, _properties.properties()->get_id());
@@ -422,7 +426,7 @@ void create_table_statement::raw_statement::add_column_alias(::shared_ptr<column
 // legal but restricted by the configuration. Checks for other of errors
 // in the table's options are done elsewhere.
 std::optional<sstring> check_restricted_table_properties(
-    query_processor& qp,
+    data_dictionary::database db,
     std::optional<schema_ptr> schema,
     const sstring& keyspace, const sstring& table,
     const cf_prop_defs& cfprops)
@@ -453,7 +457,7 @@ std::optional<sstring> check_restricted_table_properties(
         std::map<sstring, sstring> options = (strategy) ? cfprops.get_compaction_type_options() : (*schema)->compaction_strategy_options();
         sstables::time_window_compaction_strategy_options twcs_options(options);
         long ttl = (cfprops.has_property(cf_prop_defs::KW_DEFAULT_TIME_TO_LIVE)) ? cfprops.get_default_time_to_live() : current_ttl.count();
-        auto max_windows = qp.db().get_config().twcs_max_window_count();
+        auto max_windows = db.get_config().twcs_max_window_count();
 
         // It may happen that an user tries to update an unrelated table property. Allow the request through.
         if (!cfprops.has_property(cf_prop_defs::KW_DEFAULT_TIME_TO_LIVE) && !strategy) {
@@ -473,7 +477,7 @@ std::optional<sstring> check_restricted_table_properties(
                                                    "highly discouraged.", ttl, twcs_options.get_sstable_window_size().count(), window_count, max_windows));
             }
         } else {
-              switch (qp.db().get_config().restrict_twcs_without_default_ttl()) {
+              switch (db.get_config().restrict_twcs_without_default_ttl()) {
               case db::tri_mode_restriction_t::mode::TRUE:
                   throw exceptions::configuration_exception(
                       "TimeWindowCompactionStrategy tables without a strict default_time_to_live setting "
@@ -492,18 +496,6 @@ std::optional<sstring> check_restricted_table_properties(
     return std::nullopt;
 }
 
-future<::shared_ptr<messages::result_message>>
-create_table_statement::execute(query_processor& qp, service::query_state& state, const query_options& options, std::optional<service::group0_guard> guard) const {
-    std::optional<sstring> warning = check_restricted_table_properties(qp, std::nullopt, keyspace(), column_family(), *_properties);
-    return schema_altering_statement::execute(qp, state, options, std::move(guard)).then([warning = std::move(warning)] (::shared_ptr<messages::result_message> msg) {
-        if (warning) {
-            msg->add_warning(*warning);
-            mylogger.warn("{}", *warning);
-        }
-        return msg;
-    });
-}
-
 }
 
 }

cql3/statements/create_table_statement.hh

@@ -77,9 +77,6 @@ public:
 
     virtual future<> grant_permissions_to_creator(const service::client_state&) const override;
 
-    virtual future<::shared_ptr<messages::result_message>>
-    execute(query_processor& qp, service::query_state& state, const query_options& options, std::optional<service::group0_guard> guard) const override;
-
     schema_ptr get_cf_meta_data(const data_dictionary::database) const;
 
     class raw_statement;
@@ -127,7 +124,7 @@ public:
 };
 
 std::optional<sstring> check_restricted_table_properties(
-    query_processor& qp,
+    data_dictionary::database db,
     std::optional<schema_ptr> schema,
     const sstring& keyspace, const sstring& table,
     const cf_prop_defs& cfprops);

test/boost/cql_query_test.cc

@@ -103,15 +103,17 @@ SEASTAR_TEST_CASE(test_create_twcs_table_no_ttl) {
         e.execute_cql("ALTER TABLE tbl WITH default_time_to_live=60;").get();
         // LiveUpdate and enforce TTL to be defined
         e.execute_cql("UPDATE system.config SET value='true' WHERE name='restrict_twcs_without_default_ttl';").get();
-        assert_that_failed(e.execute_cql("CREATE TABLE tbl2 (a int, b int, PRIMARY KEY (a)) WITH "
-                      "compaction = {'class': 'TimeWindowCompactionStrategy', "
-                      "'compaction_window_size': '1', 'compaction_window_unit': 'MINUTES'};"));
+        // default_time_to_live option is required
+        BOOST_REQUIRE_THROW(e.execute_cql("CREATE TABLE tbl2 (a int, b int, PRIMARY KEY (a)) WITH "
+                    "compaction = {'class': 'TimeWindowCompactionStrategy', "
+                    "'compaction_window_size': '1', 'compaction_window_unit': 'MINUTES'};").get(), exceptions::configuration_exception);
         e.execute_cql("CREATE TABLE tbl2 (a int, b int, PRIMARY KEY (a)) WITH "
                       "compaction = {'class': 'TimeWindowCompactionStrategy', "
                       "'compaction_window_size': '1', 'compaction_window_unit': 'MINUTES'} AND "
                       "default_time_to_live=60").get();
         BOOST_REQUIRE(e.local_db().has_schema("ks", "tbl2"));
-        assert_that_failed(e.execute_cql("ALTER TABLE tbl WITH default_time_to_live=0;"));
+        // default_time_to_live option must not be set to 0.
+        BOOST_REQUIRE_THROW(e.execute_cql("ALTER TABLE tbl WITH default_time_to_live=0;").get(), exceptions::configuration_exception);
         // LiveUpdate and disable the check, then try table creation again
         e.execute_cql("UPDATE system.config SET value='false' WHERE name='restrict_twcs_without_default_ttl';").get();
         e.execute_cql("CREATE TABLE tbl3 (a int, b int, PRIMARY KEY (a)) WITH "
@@ -130,10 +132,10 @@ SEASTAR_TEST_CASE(test_twcs_max_window) {
         // Hardcode restriction to max 10 windows
         e.execute_cql("UPDATE system.config SET value='10' WHERE name='twcs_max_window_count';").get();
         // Creating a TWCS table with a large number of windows/buckets should fail
-        assert_that_failed(e.execute_cql("CREATE TABLE tbl (a int, b int, PRIMARY KEY (a)) WITH "
-                      "compaction = {'class': 'TimeWindowCompactionStrategy', "
-                      "'compaction_window_size': '1', 'compaction_window_unit': 'HOURS'} "
-                      "AND default_time_to_live=86400;"));
+        BOOST_REQUIRE_THROW(e.execute_cql("CREATE TABLE tbl (a int, b int, PRIMARY KEY (a)) WITH "
+                        "compaction = {'class': 'TimeWindowCompactionStrategy', "
+                        "'compaction_window_size': '1', 'compaction_window_unit': 'HOURS'} "
+                        "AND default_time_to_live=86400;").get(), exceptions::configuration_exception);
         // However the creation of a table within bounds should succeed
         e.execute_cql("CREATE TABLE tbl (a int, b int, PRIMARY KEY (a)) WITH "
                       "compaction = {'class': 'TimeWindowCompactionStrategy', "
@@ -168,19 +170,19 @@ SEASTAR_TEST_CASE(test_twcs_restrictions_mixed) {
         // Cenario 3: STCS->TWCS with large TTL value
         e.execute_cql("CREATE TABLE tbl3 (a int PRIMARY KEY, b int) WITH default_time_to_live = 8640000;").get();
         BOOST_REQUIRE(e.local_db().has_schema("ks", "tbl3"));
-        assert_that_failed(e.execute_cql("ALTER TABLE tbl3 WITH compaction = {'class': 'TimeWindowCompactionStrategy'};"));
+        BOOST_REQUIRE_THROW(e.execute_cql("ALTER TABLE tbl3 WITH compaction = {'class': 'TimeWindowCompactionStrategy'};").get(), exceptions::configuration_exception);
 
         // Cenario 4: TWCS table with small to large TTL
         e.execute_cql("CREATE TABLE tbl4 (a int PRIMARY KEY, b int) WITH compaction = "
                       "{'class': 'TimeWindowCompactionStrategy'} AND default_time_to_live = 60;").get();
         BOOST_REQUIRE(e.local_db().has_schema("ks", "tbl4"));
-        assert_that_failed(e.execute_cql("ALTER TABLE tbl4 WITH default_time_to_live = 86400000;"));
+        BOOST_REQUIRE_THROW(e.execute_cql("ALTER TABLE tbl4 WITH default_time_to_live = 86400000;").get(), exceptions::configuration_exception);
 
         // Cenario 5: No TTL TWCS to large TTL and then small TTL
         e.execute_cql("CREATE TABLE tbl5 (a int PRIMARY KEY, b int) WITH compaction = "
                       "{'class': 'TimeWindowCompactionStrategy'}").get();
         BOOST_REQUIRE(e.local_db().has_schema("ks", "tbl5"));
-        assert_that_failed(e.execute_cql("ALTER TABLE tbl5 WITH default_time_to_live = 86400000;"));
+        BOOST_REQUIRE_THROW(e.execute_cql("ALTER TABLE tbl5 WITH default_time_to_live = 86400000;").get(), exceptions::configuration_exception);
         e.execute_cql("ALTER TABLE tbl5 WITH default_time_to_live = 60;").get();
 
         // Cenario 6: twcs_max_window_count LiveUpdate - Decrease TTL
@@ -216,29 +218,29 @@ SEASTAR_TEST_CASE(test_twcs_restrictions_mixed) {
         // Cenario 10: Large TTL STCS table, fail to switch to TWCS with no TTL
         e.execute_cql("CREATE TABLE tbl10 (a int PRIMARY KEY, b int) WITH default_time_to_live = 8640000;").get();
         BOOST_REQUIRE(e.local_db().has_schema("ks", "tbl10"));
-        assert_that_failed(e.execute_cql("ALTER TABLE tbl10 WITH compaction = {'class': 'TimeWindowCompactionStrategy'};"));
+        BOOST_REQUIRE_THROW(e.execute_cql("ALTER TABLE tbl10 WITH compaction = {'class': 'TimeWindowCompactionStrategy'};").get(), exceptions::configuration_exception);
         e.execute_cql("ALTER TABLE tbl10 WITH compaction = {'class': 'TimeWindowCompactionStrategy'} AND default_time_to_live = 0;").get();
 
         // Cenario 11: Ensure default_time_to_live updates reference existing table properties
         e.execute_cql("CREATE TABLE tbl11 (a int PRIMARY KEY, b int) WITH compaction = "
                       "{'class': 'TimeWindowCompactionStrategy', 'compaction_window_size': '1', "
                       "'compaction_window_unit': 'MINUTES'} AND default_time_to_live=3000;").get();
         BOOST_REQUIRE(e.local_db().has_schema("ks", "tbl11"));
-       assert_that_failed(e.execute_cql("ALTER TABLE tbl11 WITH default_time_to_live=3600;"));
+       BOOST_REQUIRE_THROW(e.execute_cql("ALTER TABLE tbl11 WITH default_time_to_live=3600;").get(), exceptions::configuration_exception);
        e.execute_cql("ALTER TABLE tbl11 WITH compaction = {'class': 'TimeWindowCompactionStrategy', "
                      "'compaction_window_size': '2', 'compaction_window_unit': 'MINUTES'};").get();
        e.execute_cql("ALTER TABLE tbl11 WITH default_time_to_live=3600;").get();
 
        // Cenario 12: Ensure that window sizes <= 0 are forbidden
-       assert_that_failed(e.execute_cql("CREATE TABLE tbl12 (a int PRIMARY KEY, b int) WITH compaction = "
-                          "{'class': 'TimeWindowCompactionStrategy', 'compaction_window_size': '0'};"));
-       assert_that_failed(e.execute_cql("CREATE TABLE tbl12 (a int PRIMARY KEY, b int) WITH compaction = "
-                          "{'class': 'TimeWindowCompactionStrategy', 'compaction_window_size': -65535};"));
+       BOOST_REQUIRE_THROW(e.execute_cql("CREATE TABLE tbl12 (a int PRIMARY KEY, b int) WITH compaction = "
+                          "{'class': 'TimeWindowCompactionStrategy', 'compaction_window_size': '0'};").get(), exceptions::configuration_exception);
+       BOOST_REQUIRE_THROW(e.execute_cql("CREATE TABLE tbl12 (a int PRIMARY KEY, b int) WITH compaction = "
+                          "{'class': 'TimeWindowCompactionStrategy', 'compaction_window_size': -65535};").get(), exceptions::configuration_exception);
        e.execute_cql("CREATE TABLE tbl12 (a int PRIMARY KEY, b int) WITH compaction = "
                      "{'class': 'TimeWindowCompactionStrategy', 'compaction_window_size': 1};").get();
        BOOST_REQUIRE(e.local_db().has_schema("ks", "tbl12"));
-       assert_that_failed(e.execute_cql("ALTER TABLE tbl12 WITH compaction = {'class': 'TimeWindowCompactionStrategy', "
-                          "'compaction_window_size': 0};"));
+       BOOST_REQUIRE_THROW(e.execute_cql("ALTER TABLE tbl12 WITH compaction = {'class': 'TimeWindowCompactionStrategy', "
+                          "'compaction_window_size': 0};").get(), exceptions::configuration_exception);
     });
 }