From d998f066332c48c48716a3a778e1af0ba111e4c4 Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Sun, 15 Apr 2018 19:35:55 +0300 Subject: [PATCH 1/4] cql: schema_altering_statement: make execute() and execute_internal() equivalent To get rid of execute_internal(), make the normal execute() equivalent and call it instead of having two different paths. --- cql3/statements/schema_altering_statement.cc | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/cql3/statements/schema_altering_statement.cc b/cql3/statements/schema_altering_statement.cc index 6818c88429..56854f1d00 100644 --- a/cql3/statements/schema_altering_statement.cc +++ b/cql3/statements/schema_altering_statement.cc @@ -109,8 +109,12 @@ schema_altering_statement::execute0(service::storage_proxy& proxy, service::quer future<::shared_ptr> schema_altering_statement::execute(service::storage_proxy& proxy, service::query_state& state, const query_options& options) { - return execute0(proxy, state, options, false).then([this, &state](::shared_ptr result) { - return grant_permissions_to_creator(state.get_client_state()).then([result = std::move(result)] { + bool internal = state.get_client_state().is_internal(); + return execute0(proxy, state, options, internal).then([this, &state, internal](::shared_ptr result) { + auto permissions_granted_fut = internal + ? make_ready_future<>() + : grant_permissions_to_creator(state.get_client_state()); + return permissions_granted_fut.then([result = std::move(result)] { return result; }); }); @@ -118,7 +122,7 @@ schema_altering_statement::execute(service::storage_proxy& proxy, service::query future<::shared_ptr> schema_altering_statement::execute_internal(service::storage_proxy& proxy, service::query_state& state, const query_options& options) { - return execute0(proxy, state, options, true); + return execute(proxy, state, options); } } From eb19798f996f57b4bffe1e80bd1b4c65800b76aa Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Sun, 15 Apr 2018 20:31:34 +0300 Subject: [PATCH 2/4] cql: select_statement: make execute() and execute_internal() equivalent execute_internal(), for some code paths, differs from execute by the following: 1. it uses CL_ONE unconditionally 2. it has no query timeout 3. it doesn't use execution stages for other code paths, it just calls execute. As preparation for getting rid of execute_internal(), unify the two code paths. Commit 4859b759b909f93 caused the consistency level and timeouts to be provided by the caller, so using the caller provided parameters instead of overriding them does not change behavior. --- cql3/statements/select_statement.cc | 36 +---------------------------- 1 file changed, 1 insertion(+), 35 deletions(-) diff --git a/cql3/statements/select_statement.cc b/cql3/statements/select_statement.cc index c1c3087155..7c91724581 100644 --- a/cql3/statements/select_statement.cc +++ b/cql3/statements/select_statement.cc @@ -537,41 +537,7 @@ select_statement::execute_internal(service::storage_proxy& proxy, service::query_state& state, const query_options& options) { - if (options.get_specific_options().page_size > 0) { - // need page, use regular execute - return do_execute(proxy, state, options); - } - int32_t limit = get_limit(options); - auto now = gc_clock::now(); - auto command = ::make_lw_shared(_schema->id(), _schema->version(), - make_partition_slice(options), limit, now, std::experimental::nullopt, query::max_partitions, utils::UUID(), options.get_timestamp(state)); - auto partition_ranges = _restrictions->get_partition_key_ranges(options); - - tracing::add_table_name(state.get_trace_state(), keyspace(), column_family()); - - ++_stats.reads; - - if (needs_post_query_ordering() && _limit) { - return do_with(std::move(partition_ranges), [this, &proxy, &state, command] (auto& prs) { - assert(command->partition_limit == query::max_partitions); - query::result_merger merger(command->row_limit * prs.size(), query::max_partitions); - return map_reduce(prs.begin(), prs.end(), [this, &proxy, &state, command] (auto& pr) { - dht::partition_range_vector prange { pr }; - auto cmd = ::make_lw_shared(*command); - return proxy.query(_schema, cmd, std::move(prange), db::consistency_level::ONE, {db::no_timeout, state.get_trace_state(), - }).then([] (service::storage_proxy::coordinator_query_result qr) { - return std::move(qr.query_result); - }); - }, std::move(merger)); - }).then([command, this, &options, now] (auto result) { - return this->process_results(std::move(result), command, options, now); - }).finally([command] { }); - } else { - auto query = proxy.query(_schema, command, std::move(partition_ranges), db::consistency_level::ONE, {db::no_timeout, state.get_trace_state()}); - return query.then([command, this, &options, now] (service::storage_proxy::coordinator_query_result qr) { - return this->process_results(std::move(qr.query_result), command, options, now); - }).finally([command] {}); - } + return execute(proxy, state, options); } shared_ptr From c8a66efb6a483cb4f595fdbbf956538eacefcebb Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Sun, 15 Apr 2018 20:36:36 +0300 Subject: [PATCH 3/4] cql: query_processor: don't call cql_statement::execute_internal() any more All cql_statement::execute_internal() overrides now either throw or call execute(). Since we shouldn't be calling the throwing overrides internally, we can safely call execute() instead. This allows us to get rid of execute_internal(). --- cql3/query_processor.cc | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/cql3/query_processor.cc b/cql3/query_processor.cc index 6f11251922..3c2e2b1424 100644 --- a/cql3/query_processor.cc +++ b/cql3/query_processor.cc @@ -291,12 +291,7 @@ query_processor::process_authorized_statement(const ::shared_ptr statement->validate(_proxy, client_state); - auto fut = make_ready_future<::shared_ptr>(); - if (client_state.is_internal()) { - fut = statement->execute_internal(_proxy, query_state, options); - } else { - fut = statement->execute(_proxy, query_state, options); - } + auto fut = statement->execute(_proxy, query_state, options); return fut.then([statement] (auto msg) { if (msg) { @@ -522,7 +517,7 @@ future<> query_processor::for_each_cql_result( future<::shared_ptr> query_processor::execute_paged_internal(::shared_ptr state) { - return state->p->statement->execute_internal(_proxy, *_internal_state, *state->opts).then( + return state->p->statement->execute(_proxy, *_internal_state, *state->opts).then( [state, this](::shared_ptr msg) mutable { class visitor : public result_message::visitor_base { ::shared_ptr _state; @@ -563,7 +558,7 @@ query_processor::execute_internal( const std::initializer_list& values) { query_options opts = make_internal_options(p, values, db::consistency_level::ONE, infinite_timeout_config); return do_with(std::move(opts), [this, p = std::move(p)](auto& opts) { - return p->statement->execute_internal( + return p->statement->execute( _proxy, *_internal_state, opts).then([&opts, stmt = p->statement](auto msg) { From b70febe2463d44ccd040168678afa96ab1e8f391 Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Sun, 15 Apr 2018 20:38:45 +0300 Subject: [PATCH 4/4] cql: cql_statement: remove execute_internal() With no callers, it can be safely removed. --- cql3/cql_statement.hh | 8 -------- cql3/statements/authentication_statement.cc | 7 ------- cql3/statements/authentication_statement.hh | 3 --- cql3/statements/authorization_statement.cc | 7 ------- cql3/statements/authorization_statement.hh | 3 --- cql3/statements/batch_statement.cc | 17 ----------------- cql3/statements/batch_statement.hh | 4 ---- cql3/statements/modification_statement.cc | 20 -------------------- cql3/statements/modification_statement.hh | 3 --- cql3/statements/schema_altering_statement.cc | 5 ----- cql3/statements/schema_altering_statement.hh | 3 --- cql3/statements/select_statement.cc | 8 -------- cql3/statements/select_statement.hh | 3 --- cql3/statements/truncate_statement.cc | 6 ------ cql3/statements/truncate_statement.hh | 3 --- cql3/statements/use_statement.cc | 6 ------ cql3/statements/use_statement.hh | 3 --- 17 files changed, 109 deletions(-) diff --git a/cql3/cql_statement.hh b/cql3/cql_statement.hh index 9c1fdd2475..33874683ad 100644 --- a/cql3/cql_statement.hh +++ b/cql3/cql_statement.hh @@ -98,14 +98,6 @@ public: virtual future<::shared_ptr> execute(service::storage_proxy& proxy, service::query_state& state, const query_options& options) = 0; - /** - * Variant of execute used for internal query against the system tables, and thus only query the local node = 0. - * - * @param state the current query state - */ - virtual future<::shared_ptr> - execute_internal(service::storage_proxy& proxy, service::query_state& state, const query_options& options) = 0; - virtual bool uses_function(const sstring& ks_name, const sstring& function_name) const = 0; virtual bool depends_on_keyspace(const sstring& ks_name) const = 0; diff --git a/cql3/statements/authentication_statement.cc b/cql3/statements/authentication_statement.cc index 5c9a3c2eda..e94c39e2c3 100644 --- a/cql3/statements/authentication_statement.cc +++ b/cql3/statements/authentication_statement.cc @@ -74,10 +74,3 @@ void cql3::statements::authentication_statement::validate( future<> cql3::statements::authentication_statement::check_access(const service::client_state& state) { return make_ready_future<>(); } - -future<::shared_ptr> cql3::statements::authentication_statement::execute_internal( - service::storage_proxy& proxy, - service::query_state& state, const query_options& options) { - // Internal queries are exclusively on the system keyspace and makes no sense here - throw std::runtime_error("unsupported operation"); -} diff --git a/cql3/statements/authentication_statement.hh b/cql3/statements/authentication_statement.hh index 6dcaefafc1..3300edc9ab 100644 --- a/cql3/statements/authentication_statement.hh +++ b/cql3/statements/authentication_statement.hh @@ -67,9 +67,6 @@ public: future<> check_access(const service::client_state& state) override; void validate(service::storage_proxy&, const service::client_state& state) override; - - future<::shared_ptr> - execute_internal(service::storage_proxy& proxy, service::query_state& state, const query_options& options) override; }; } diff --git a/cql3/statements/authorization_statement.cc b/cql3/statements/authorization_statement.cc index 3fa73b43f1..cbbd89c016 100644 --- a/cql3/statements/authorization_statement.cc +++ b/cql3/statements/authorization_statement.cc @@ -75,13 +75,6 @@ future<> cql3::statements::authorization_statement::check_access(const service:: return make_ready_future<>(); } -future<::shared_ptr> cql3::statements::authorization_statement::execute_internal( - service::storage_proxy& proxy, - service::query_state& state, const query_options& options) { - // Internal queries are exclusively on the system keyspace and makes no sense here - throw std::runtime_error("unsupported operation"); -} - void cql3::statements::authorization_statement::maybe_correct_resource(auth::resource& resource, const service::client_state& state) { if (resource.kind() == auth::resource_kind::data) { const auto data_view = auth::data_resource_view(resource); diff --git a/cql3/statements/authorization_statement.hh b/cql3/statements/authorization_statement.hh index 72c204fb67..a86c7bf7f4 100644 --- a/cql3/statements/authorization_statement.hh +++ b/cql3/statements/authorization_statement.hh @@ -72,9 +72,6 @@ public: void validate(service::storage_proxy&, const service::client_state& state) override; - future<::shared_ptr> - execute_internal(service::storage_proxy& proxy, service::query_state& state, const query_options& options) override; - protected: static void maybe_correct_resource(auth::resource&, const service::client_state&); }; diff --git a/cql3/statements/batch_statement.cc b/cql3/statements/batch_statement.cc index 8916781b95..35d0458732 100644 --- a/cql3/statements/batch_statement.cc +++ b/cql3/statements/batch_statement.cc @@ -403,23 +403,6 @@ future> batch_statement::exe #endif } -future> batch_statement::execute_internal( - service::storage_proxy& proxy, - service::query_state& query_state, const query_options& options) -{ - throw std::runtime_error(sprint("%s not implemented", __PRETTY_FUNCTION__)); -#if 0 - assert !hasConditions; - for (IMutation mutation : getMutations(BatchQueryOptions.withoutPerStatementVariables(options), true, queryState.getTimestamp())) - { - // We don't use counters internally. - assert mutation instanceof Mutation; - ((Mutation) mutation).apply(); - } - return null; -#endif -} - namespace raw { std::unique_ptr diff --git a/cql3/statements/batch_statement.hh b/cql3/statements/batch_statement.hh index f31d24e79f..0aeb98d0e4 100644 --- a/cql3/statements/batch_statement.hh +++ b/cql3/statements/batch_statement.hh @@ -154,10 +154,6 @@ private: const query_options& options, service::query_state& state); public: - virtual future> execute_internal( - service::storage_proxy& proxy, - service::query_state& query_state, const query_options& options) override; - // FIXME: no cql_statement::to_string() yet #if 0 sstring to_string() const { diff --git a/cql3/statements/modification_statement.cc b/cql3/statements/modification_statement.cc index f68bee6a6e..ab6d91f661 100644 --- a/cql3/statements/modification_statement.cc +++ b/cql3/statements/modification_statement.cc @@ -438,26 +438,6 @@ modification_statement::execute_with_condition(service::storage_proxy& proxy, se #endif } -future<::shared_ptr> -modification_statement::execute_internal(service::storage_proxy& proxy, service::query_state& qs, const query_options& options) { - if (has_conditions()) { - throw exceptions::unsupported_operation_exception(); - } - - tracing::add_table_name(qs.get_trace_state(), keyspace(), column_family()); - - inc_cql_stats(); - - return get_mutations(proxy, options, true, options.get_timestamp(qs), qs.get_trace_state()).then( - [&proxy] (auto mutations) { - return proxy.mutate_locally(std::move(mutations)); - }).then( - [] { - return make_ready_future<::shared_ptr>( - ::shared_ptr {}); - }); -} - void modification_statement::process_where_clause(database& db, std::vector where_clause, ::shared_ptr names) { _restrictions = ::make_shared( diff --git a/cql3/statements/modification_statement.hh b/cql3/statements/modification_statement.hh index e75f9e7d88..01647a13d3 100644 --- a/cql3/statements/modification_statement.hh +++ b/cql3/statements/modification_statement.hh @@ -217,9 +217,6 @@ public: virtual future<::shared_ptr> execute(service::storage_proxy& proxy, service::query_state& qs, const query_options& options) override; - virtual future<::shared_ptr> - execute_internal(service::storage_proxy& proxy, service::query_state& qs, const query_options& options) override; - private: future<> execute_without_condition(service::storage_proxy& proxy, service::query_state& qs, const query_options& options); diff --git a/cql3/statements/schema_altering_statement.cc b/cql3/statements/schema_altering_statement.cc index 56854f1d00..78f1aacfcc 100644 --- a/cql3/statements/schema_altering_statement.cc +++ b/cql3/statements/schema_altering_statement.cc @@ -120,11 +120,6 @@ schema_altering_statement::execute(service::storage_proxy& proxy, service::query }); } -future<::shared_ptr> -schema_altering_statement::execute_internal(service::storage_proxy& proxy, service::query_state& state, const query_options& options) { - return execute(proxy, state, options); -} - } } diff --git a/cql3/statements/schema_altering_statement.hh b/cql3/statements/schema_altering_statement.hh index c32429349e..d857f94d09 100644 --- a/cql3/statements/schema_altering_statement.hh +++ b/cql3/statements/schema_altering_statement.hh @@ -93,9 +93,6 @@ protected: virtual future<::shared_ptr> execute(service::storage_proxy& proxy, service::query_state& state, const query_options& options) override; - - virtual future<::shared_ptr> - execute_internal(service::storage_proxy&, service::query_state& state, const query_options& options) override; }; } diff --git a/cql3/statements/select_statement.cc b/cql3/statements/select_statement.cc index 7c91724581..3719794f42 100644 --- a/cql3/statements/select_statement.cc +++ b/cql3/statements/select_statement.cc @@ -532,14 +532,6 @@ select_statement::execute(service::storage_proxy& proxy, }); } -future<::shared_ptr> -select_statement::execute_internal(service::storage_proxy& proxy, - service::query_state& state, - const query_options& options) -{ - return execute(proxy, state, options); -} - shared_ptr select_statement::process_results(foreign_ptr> results, lw_shared_ptr cmd, diff --git a/cql3/statements/select_statement.hh b/cql3/statements/select_statement.hh index c311c766b9..f9d586dcc9 100644 --- a/cql3/statements/select_statement.hh +++ b/cql3/statements/select_statement.hh @@ -117,9 +117,6 @@ public: virtual future<::shared_ptr> execute(service::storage_proxy& proxy, service::query_state& state, const query_options& options) override; - virtual future<::shared_ptr> execute_internal(service::storage_proxy& proxy, - service::query_state& state, const query_options& options) override; - future<::shared_ptr> execute(service::storage_proxy& proxy, lw_shared_ptr cmd, dht::partition_range_vector&& partition_ranges, service::query_state& state, const query_options& options, gc_clock::time_point now); diff --git a/cql3/statements/truncate_statement.cc b/cql3/statements/truncate_statement.cc index 6dc9455e21..6779f9513e 100644 --- a/cql3/statements/truncate_statement.cc +++ b/cql3/statements/truncate_statement.cc @@ -106,12 +106,6 @@ truncate_statement::execute(service::storage_proxy& proxy, service::query_state& }); } -future<::shared_ptr> -truncate_statement::execute_internal(service::storage_proxy& proxy, service::query_state& state, const query_options& options) -{ - throw std::runtime_error("unsupported operation"); -} - } } diff --git a/cql3/statements/truncate_statement.hh b/cql3/statements/truncate_statement.hh index 870a529de9..fcbcb4ddfb 100644 --- a/cql3/statements/truncate_statement.hh +++ b/cql3/statements/truncate_statement.hh @@ -70,9 +70,6 @@ public: virtual future<::shared_ptr> execute(service::storage_proxy& proxy, service::query_state& state, const query_options& options) override; - - virtual future<::shared_ptr> - execute_internal(service::storage_proxy& proxy, service::query_state& state, const query_options& options) override; }; } diff --git a/cql3/statements/use_statement.cc b/cql3/statements/use_statement.cc index 394e3376ea..f358964b40 100644 --- a/cql3/statements/use_statement.cc +++ b/cql3/statements/use_statement.cc @@ -105,12 +105,6 @@ use_statement::execute(service::storage_proxy& proxy, service::query_state& stat return make_ready_future<::shared_ptr>(result); } -future<::shared_ptr> -use_statement::execute_internal(service::storage_proxy& proxy, service::query_state& state, const query_options& options) { - // Internal queries are exclusively on the system keyspace and 'use' is thus useless - throw std::runtime_error("unsupported operation"); -} - } } diff --git a/cql3/statements/use_statement.hh b/cql3/statements/use_statement.hh index ae430b192d..a866bbb90d 100644 --- a/cql3/statements/use_statement.hh +++ b/cql3/statements/use_statement.hh @@ -71,9 +71,6 @@ public: virtual future<::shared_ptr> execute(service::storage_proxy& proxy, service::query_state& state, const query_options& options) override; - - virtual future<::shared_ptr> - execute_internal(service::storage_proxy& proxy, service::query_state& state, const query_options& options) override; }; }