Skip to content

Commit

Permalink
query_processor: remove default internal query caching behavior
Browse files Browse the repository at this point in the history 
When executing internal queries, it is important that the developer
will decide if to cache the query internally or not since internal
queries are cached indefinitely. Also important is that the programmer
will be aware if caching is going to happen or not.
The code contained two "groups" of `query_processor::execute_internal`,
one group has caching by default and the other doesn't.
Here we add overloads to eliminate default values for caching behaviour,
forcing an explicit parameter for the caching values.
All the call sites were changed to reflect the original caching default
that was there.

Signed-off-by: Eliran Sinvani <[email protected]>
  • Loading branch information
Eliran Sinvani committed May 1, 2022
1 parent 38b7ebf commit e0c7178
Show file tree
Hide file tree
Showing 17 changed files with 156 additions and 114 deletions.
18 changes: 12 additions & 6 deletions auth/default_authorizer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,8 @@ future<> default_authorizer::migrate_legacy_metadata() const {

return _qp.execute_internal(
query,
db::consistency_level::LOCAL_ONE).then([this](::shared_ptr<cql3::untyped_result_set> results) {
db::consistency_level::LOCAL_ONE,
cql3::query_processor::cache_internal::no).then([this](::shared_ptr<cql3::untyped_result_set> results) {
return do_for_each(*results, [this](const cql3::untyped_result_set_row& row) {
return do_with(
row.get_as<sstring>("username"),
Expand Down Expand Up @@ -168,7 +169,8 @@ default_authorizer::authorize(const role_or_anonymous& maybe_role, const resourc
return _qp.execute_internal(
query,
db::consistency_level::LOCAL_ONE,
{*maybe_role.name, r.name()}).then([](::shared_ptr<cql3::untyped_result_set> results) {
{*maybe_role.name, r.name()},
cql3::query_processor::cache_internal::no).then([](::shared_ptr<cql3::untyped_result_set> results) {
if (results->empty()) {
return permissions::NONE;
}
Expand Down Expand Up @@ -197,7 +199,8 @@ default_authorizer::modify(
query,
db::consistency_level::ONE,
internal_distributed_query_state(),
{permissions::to_strings(set), sstring(role_name), resource.name()}).discard_result();
{permissions::to_strings(set), sstring(role_name), resource.name()},
cql3::query_processor::cache_internal::no).discard_result();
});
}

Expand Down Expand Up @@ -249,7 +252,8 @@ future<> default_authorizer::revoke_all(std::string_view role_name) const {
query,
db::consistency_level::ONE,
internal_distributed_query_state(),
{sstring(role_name)}).discard_result().handle_exception([role_name](auto ep) {
{sstring(role_name)},
cql3::query_processor::cache_internal::no).discard_result().handle_exception([role_name](auto ep) {
try {
std::rethrow_exception(ep);
} catch (exceptions::request_execution_exception& e) {
Expand All @@ -268,7 +272,8 @@ future<> default_authorizer::revoke_all(const resource& resource) const {
return _qp.execute_internal(
query,
db::consistency_level::LOCAL_ONE,
{resource.name()}).then_wrapped([this, resource](future<::shared_ptr<cql3::untyped_result_set>> f) {
{resource.name()},
cql3::query_processor::cache_internal::no).then_wrapped([this, resource](future<::shared_ptr<cql3::untyped_result_set>> f) {
try {
auto res = f.get0();
return parallel_for_each(
Expand All @@ -284,7 +289,8 @@ future<> default_authorizer::revoke_all(const resource& resource) const {
return _qp.execute_internal(
query,
db::consistency_level::LOCAL_ONE,
{r.get_as<sstring>(ROLE_NAME), resource.name()}).discard_result().handle_exception(
{r.get_as<sstring>(ROLE_NAME), resource.name()},
cql3::query_processor::cache_internal::no).discard_result().handle_exception(
[resource](auto ep) {
try {
std::rethrow_exception(ep);
Expand Down
18 changes: 12 additions & 6 deletions auth/password_authenticator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ future<> password_authenticator::migrate_legacy_metadata() const {
return _qp.execute_internal(
query,
db::consistency_level::QUORUM,
internal_distributed_query_state()).then([this](::shared_ptr<cql3::untyped_result_set> results) {
internal_distributed_query_state(),
cql3::query_processor::cache_internal::no).then([this](::shared_ptr<cql3::untyped_result_set> results) {
return do_for_each(*results, [this](const cql3::untyped_result_set_row& row) {
auto username = row.get_as<sstring>("username");
auto salted_hash = row.get_as<sstring>(SALTED_HASH);
Expand All @@ -96,7 +97,8 @@ future<> password_authenticator::migrate_legacy_metadata() const {
update_row_query(),
consistency_for_user(username),
internal_distributed_query_state(),
{std::move(salted_hash), username}).discard_result();
{std::move(salted_hash), username},
cql3::query_processor::cache_internal::no).discard_result();
}).finally([results] {});
}).then([] {
plogger.info("Finished migrating legacy authentication metadata.");
Expand All @@ -113,7 +115,8 @@ future<> password_authenticator::create_default_if_missing() const {
update_row_query(),
db::consistency_level::QUORUM,
internal_distributed_query_state(),
{passwords::hash(DEFAULT_USER_PASSWORD, rng_for_salt), DEFAULT_USER_NAME}).then([](auto&&) {
{passwords::hash(DEFAULT_USER_PASSWORD, rng_for_salt), DEFAULT_USER_NAME},
cql3::query_processor::cache_internal::no).then([](auto&&) {
plogger.info("Created default superuser authentication record.");
});
}
Expand Down Expand Up @@ -244,7 +247,8 @@ future<> password_authenticator::create(std::string_view role_name, const authen
update_row_query(),
consistency_for_user(role_name),
internal_distributed_query_state(),
{passwords::hash(*options.password, rng_for_salt), sstring(role_name)}).discard_result();
{passwords::hash(*options.password, rng_for_salt), sstring(role_name)},
cql3::query_processor::cache_internal::no).discard_result();
}

future<> password_authenticator::alter(std::string_view role_name, const authentication_options& options) const {
Expand All @@ -261,7 +265,8 @@ future<> password_authenticator::alter(std::string_view role_name, const authent
query,
consistency_for_user(role_name),
internal_distributed_query_state(),
{passwords::hash(*options.password, rng_for_salt), sstring(role_name)}).discard_result();
{passwords::hash(*options.password, rng_for_salt), sstring(role_name)},
cql3::query_processor::cache_internal::no).discard_result();
}

future<> password_authenticator::drop(std::string_view name) const {
Expand All @@ -273,7 +278,8 @@ future<> password_authenticator::drop(std::string_view name) const {
return _qp.execute_internal(
query, consistency_for_user(name),
internal_distributed_query_state(),
{sstring(name)}).discard_result();
{sstring(name)},
cql3::query_processor::cache_internal::no).discard_result();
}

future<custom_options> password_authenticator::query_custom_options(std::string_view role_name) const {
Expand Down
3 changes: 2 additions & 1 deletion auth/roles-metadata.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,8 @@ future<bool> any_nondefault_role_row_satisfies(
return qp.execute_internal(
query,
db::consistency_level::QUORUM,
internal_distributed_query_state()).then([&p](::shared_ptr<cql3::untyped_result_set> results) {
internal_distributed_query_state(),
cql3::query_processor::cache_internal::no).then([&p](::shared_ptr<cql3::untyped_result_set> results) {
if (results->empty()) {
return false;
}
Expand Down
3 changes: 2 additions & 1 deletion auth/service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,8 @@ future<bool> service::has_existing_legacy_users() const {

return _qp.execute_internal(
all_users_query,
db::consistency_level::QUORUM).then([](auto results) {
db::consistency_level::QUORUM,
cql3::query_processor::cache_internal::no).then([](auto results) {
return make_ready_future<bool>(!results->empty());
});
});
Expand Down
35 changes: 22 additions & 13 deletions auth/standard_role_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,8 @@ future<> standard_role_manager::create_default_role_if_missing() const {
query,
db::consistency_level::QUORUM,
internal_distributed_query_state(),
{meta::DEFAULT_SUPERUSER_NAME}).then([](auto&&) {
{meta::DEFAULT_SUPERUSER_NAME},
cql3::query_processor::cache_internal::no).then([](auto&&) {
log.info("Created default superuser role '{}'.", meta::DEFAULT_SUPERUSER_NAME);
return make_ready_future<>();
});
Expand All @@ -204,7 +205,8 @@ future<> standard_role_manager::migrate_legacy_metadata() const {
return _qp.execute_internal(
query,
db::consistency_level::QUORUM,
internal_distributed_query_state()).then([this](::shared_ptr<cql3::untyped_result_set> results) {
internal_distributed_query_state(),
cql3::query_processor::cache_internal::no).then([this](::shared_ptr<cql3::untyped_result_set> results) {
return do_for_each(*results, [this](const cql3::untyped_result_set_row& row) {
role_config config;
config.is_superuser = row.get_or<bool>("super", false);
Expand Down Expand Up @@ -309,7 +311,8 @@ standard_role_manager::alter(std::string_view role_name, const role_config_updat
meta::roles_table::role_col_name),
consistency_for_role(role_name),
internal_distributed_query_state(),
{sstring(role_name)}).discard_result();
{sstring(role_name)},
cql3::query_processor::cache_internal::no).discard_result();
});
}

Expand All @@ -328,7 +331,8 @@ future<> standard_role_manager::drop(std::string_view role_name) {
query,
consistency_for_role(role_name),
internal_distributed_query_state(),
{sstring(role_name)}).then([this, role_name](::shared_ptr<cql3::untyped_result_set> members) {
{sstring(role_name)},
cql3::query_processor::cache_internal::no).then([this, role_name](::shared_ptr<cql3::untyped_result_set> members) {
return parallel_for_each(
members->begin(),
members->end(),
Expand Down Expand Up @@ -360,7 +364,7 @@ future<> standard_role_manager::drop(std::string_view role_name) {
// Delete all attributes for that role
const auto remove_attributes_of = [this, role_name] {
static const sstring query = format("DELETE FROM {} WHERE role = ?", meta::role_attributes_table::qualified_name());
return _qp.execute_internal(query, {sstring(role_name)}).discard_result();
return _qp.execute_internal(query, {sstring(role_name)}, cql3::query_processor::cache_internal::yes).discard_result();
};

// Finally, delete the role itself.
Expand All @@ -373,7 +377,8 @@ future<> standard_role_manager::drop(std::string_view role_name) {
query,
consistency_for_role(role_name),
internal_distributed_query_state(),
{sstring(role_name)}).discard_result();
{sstring(role_name)},
cql3::query_processor::cache_internal::no).discard_result();
};

return when_all_succeed(revoke_from_members(), revoke_members_of(),
Expand Down Expand Up @@ -401,7 +406,8 @@ standard_role_manager::modify_membership(
query,
consistency_for_role(grantee_name),
internal_distributed_query_state(),
{role_set{sstring(role_name)}, sstring(grantee_name)}).discard_result();
{role_set{sstring(role_name)}, sstring(grantee_name)},
cql3::query_processor::cache_internal::no).discard_result();
};

const auto modify_role_members = [this, role_name, grantee_name, ch] {
Expand All @@ -412,15 +418,17 @@ standard_role_manager::modify_membership(
meta::role_members_table::qualified_name),
consistency_for_role(role_name),
internal_distributed_query_state(),
{sstring(role_name), sstring(grantee_name)}).discard_result();
{sstring(role_name), sstring(grantee_name)},
cql3::query_processor::cache_internal::no).discard_result();

case membership_change::remove:
return _qp.execute_internal(
format("DELETE FROM {} WHERE role = ? AND member = ?",
meta::role_members_table::qualified_name),
consistency_for_role(role_name),
internal_distributed_query_state(),
{sstring(role_name), sstring(grantee_name)}).discard_result();
{sstring(role_name), sstring(grantee_name)},
cql3::query_processor::cache_internal::no).discard_result();
}

return make_ready_future<>();
Expand Down Expand Up @@ -522,7 +530,8 @@ future<role_set> standard_role_manager::query_all() {
return _qp.execute_internal(
query,
db::consistency_level::QUORUM,
internal_distributed_query_state()).then([](::shared_ptr<cql3::untyped_result_set> results) {
internal_distributed_query_state(),
cql3::query_processor::cache_internal::no).then([](::shared_ptr<cql3::untyped_result_set> results) {
role_set roles;

std::transform(
Expand Down Expand Up @@ -557,7 +566,7 @@ future<bool> standard_role_manager::can_login(std::string_view role_name) {

future<std::optional<sstring>> standard_role_manager::get_attribute(std::string_view role_name, std::string_view attribute_name) {
static const sstring query = format("SELECT name, value FROM {} WHERE role = ? AND name = ?", meta::role_attributes_table::qualified_name());
return _qp.execute_internal(query, {sstring(role_name), sstring(attribute_name)}).then([] (shared_ptr<cql3::untyped_result_set> result_set) {
return _qp.execute_internal(query, {sstring(role_name), sstring(attribute_name)}, cql3::query_processor::cache_internal::yes).then([] (shared_ptr<cql3::untyped_result_set> result_set) {
if (!result_set->empty()) {
const cql3::untyped_result_set_row &row = result_set->one();
return std::optional<sstring>(row.get_as<sstring>("value"));
Expand Down Expand Up @@ -590,7 +599,7 @@ future<> standard_role_manager::set_attribute(std::string_view role_name, std::s
if (!role_exists) {
throw auth::nonexistant_role(role_name);
}
return _qp.execute_internal(query, {sstring(role_name), sstring(attribute_name), sstring(attribute_value)}).discard_result();
return _qp.execute_internal(query, {sstring(role_name), sstring(attribute_name), sstring(attribute_value)}, cql3::query_processor::cache_internal::yes).discard_result();
});
});

Expand All @@ -603,7 +612,7 @@ future<> standard_role_manager::remove_attribute(std::string_view role_name, std
if (!role_exists) {
throw auth::nonexistant_role(role_name);
}
return _qp.execute_internal(query, {sstring(role_name), sstring(attribute_name)}).discard_result();
return _qp.execute_internal(query, {sstring(role_name), sstring(attribute_name)}, cql3::query_processor::cache_internal::yes).discard_result();
});
});
}
Expand Down
42 changes: 26 additions & 16 deletions cql3/query_processor.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,17 +236,6 @@ public:
service::query_state& query_state,
query_options& options);

// NOTICE: Internal queries should be used with care, as they are expected
// to be used for local tables (e.g. from the `system` keyspace).
// Data modifications will usually be performed with consistency level ONE
// and schema changes will not be announced to other nodes.
// Because of that, changing global schema state (e.g. modifying non-local tables,
// creating namespaces, etc) is explicitly forbidden via this interface.
future<::shared_ptr<untyped_result_set>>
execute_internal(const sstring& query_string, const std::initializer_list<data_value>& values = { }) {
return execute_internal(query_string, db::consistency_level::ONE, values, cache_internal::yes);
}

statements::prepared_statement::checked_weak_ptr prepare_internal(const sstring& query);

/*!
Expand Down Expand Up @@ -300,6 +289,7 @@ public:

class cache_internal_tag;
using cache_internal = bool_class<cache_internal_tag>;

// NOTICE: Internal queries should be used with care, as they are expected
// to be used for local tables (e.g. from the `system` keyspace).
// Data modifications will usually be performed with consistency level ONE
Expand All @@ -309,15 +299,35 @@ public:
future<::shared_ptr<untyped_result_set>> execute_internal(
const sstring& query_string,
db::consistency_level,
const std::initializer_list<data_value>& = { },
cache_internal cache = cache_internal::no);
const std::initializer_list<data_value>&,
cache_internal cache);
future<::shared_ptr<untyped_result_set>> execute_internal(
const sstring& query_string,
db::consistency_level,
service::query_state& query_state,
const std::initializer_list<data_value>& = { },
cache_internal cache = cache_internal::no);

const std::initializer_list<data_value>&,
cache_internal cache);
future<::shared_ptr<untyped_result_set>> execute_internal(
const sstring& query_string,
db::consistency_level cl,
cache_internal cache) {
return execute_internal(query_string, cl, {}, cache);
}
future<::shared_ptr<untyped_result_set>> execute_internal(
const sstring& query_string,
db::consistency_level cl,
service::query_state& query_state,
cache_internal cache) {
return execute_internal(query_string, cl, query_state, {}, cache);
}
future<::shared_ptr<untyped_result_set>>
execute_internal(const sstring& query_string, const std::initializer_list<data_value>& values, cache_internal cache) {
return execute_internal(query_string, db::consistency_level::ONE, values, cache);
}
future<::shared_ptr<untyped_result_set>>
execute_internal(const sstring& query_string, cache_internal cache) {
return execute_internal(query_string, db::consistency_level::ONE, {}, cache);
}
future<::shared_ptr<untyped_result_set>> execute_with_params(
statements::prepared_statement::checked_weak_ptr p,
db::consistency_level,
Expand Down
Loading

0 comments on commit e0c7178

Please sign in to comment.