From 4e2c8feedadb402c9b172cef84e50bfdbcb14372 Mon Sep 17 00:00:00 2001 From: Xavi Garcia Date: Wed, 22 Nov 2023 15:44:59 +0100 Subject: [PATCH] rgw/sfs: Few changes around sqlite modern cpp adoption Started using move semantics for conversions (more to come). DBConn::get uses sqlite_orm's `get_storage` so we are opening the database in the same way. More blob generalisation. Signed-off-by: Xavi Garcia --- src/rgw/driver/sfs/sqlite/bindings/blob.h | 27 ++++------- src/rgw/driver/sfs/sqlite/conversion_utils.h | 45 +++++++++++-------- src/rgw/driver/sfs/sqlite/dbconn.cc | 19 +++++--- .../sfs/sqlite/objects/object_definitions.h | 8 ++-- .../driver/sfs/sqlite/sqlite_query_utils.h | 15 ++++--- src/rgw/driver/sfs/sqlite/sqlite_users.cc | 4 +- .../sfs/sqlite/users/users_definitions.h | 4 +- 7 files changed, 64 insertions(+), 58 deletions(-) diff --git a/src/rgw/driver/sfs/sqlite/bindings/blob.h b/src/rgw/driver/sfs/sqlite/bindings/blob.h index 7de28bb23abc7d..abe0b938ce1f86 100644 --- a/src/rgw/driver/sfs/sqlite/bindings/blob.h +++ b/src/rgw/driver/sfs/sqlite/bindings/blob.h @@ -23,34 +23,23 @@ #include "rgw/driver/sfs/sqlite/sqlite_orm.h" #include "rgw_common.h" -namespace blob_utils { - -template -struct has_type; - -template -struct has_type> : std::false_type {}; - -template -struct has_type> : has_type> {}; - -template -struct has_type> : std::true_type {}; +namespace sqlite_orm { -// list of types that are stored as blobs and have the encode/decode functions +// Add to this tuple all the types that you need to store in sqlite as a blob. +// Those types need to have encode/decode methods based on ceph's bufferlist. +// Also if your type has the decode/encode methods out of the ceph namespace, go +// to conversion-utils.h and add your type to the +// TypesDecodeIsNOTInCephNamespace tuple. using BlobTypes = std::tuple< rgw::sal::Attrs, ACLOwner, rgw_placement_rule, std::map, std::map, RGWUserCaps, std::list, std::map, RGWQuotaInfo, std::set, RGWBucketWebsiteConf, std::map, RGWObjectLock, rgw_sync_policy_info>; -} // namespace blob_utils - -namespace sqlite_orm { template inline constexpr bool is_sqlite_blob = - blob_utils::has_type::value; + blob_utils::has_type::value; template struct type_printer, void>::type> @@ -97,7 +86,7 @@ struct row_extractor< namespace rgw::sal::sfs::dbapi::sqlite { template struct has_sqlite_type - : blob_utils::has_type {}; + : blob_utils::has_type {}; template inline std::enable_if, int>::type bind_col_in_db( diff --git a/src/rgw/driver/sfs/sqlite/conversion_utils.h b/src/rgw/driver/sfs/sqlite/conversion_utils.h index de0f9069243805..b6fc1f2641906a 100644 --- a/src/rgw/driver/sfs/sqlite/conversion_utils.h +++ b/src/rgw/driver/sfs/sqlite/conversion_utils.h @@ -17,28 +17,37 @@ #include "rgw_acl.h" #include "rgw_common.h" -namespace rgw::sal::sfs::sqlite { +namespace blob_utils { + +template +struct has_type; -/// by default type's decode function is under the ceph namespace template -struct __ceph_ns_decode : std::true_type {}; +struct has_type> : std::false_type {}; + +template +struct has_type> : has_type> {}; + +template +struct has_type> : std::true_type {}; +} // namespace blob_utils + +namespace rgw::sal::sfs::sqlite { +// Normally the encode/decode methods for rgw types are found in the ceph +// namespace. But there are a few types where that's not true. +// This tuple lists all the types where the encode/decode methods are NOT in the +// ceph namespace. +// This is required to specify which call will need your type when encoding or +// decoding it from/to a bufferlist +using TypesDecodeIsNOTInCephNamespace = std::tuple< + RGWAccessControlPolicy, RGWQuotaInfo, RGWObjectLock, RGWUserCaps, ACLOwner, + rgw_placement_rule>; + +/// Returns if a type has its encode/decode methods in the ceph namespace. template -inline constexpr bool ceph_ns_decode = __ceph_ns_decode::value; - -// specialize the ones that are not under the ceph namespace -template <> -struct __ceph_ns_decode : std::false_type {}; -template <> -struct __ceph_ns_decode : std::false_type {}; -template <> -struct __ceph_ns_decode : std::false_type {}; -template <> -struct __ceph_ns_decode : std::false_type {}; -template <> -struct __ceph_ns_decode : std::false_type {}; -template <> -struct __ceph_ns_decode : std::false_type {}; +inline constexpr bool ceph_ns_decode = + !blob_utils::has_type::value; template void decode_blob(const BLOB_HOLDER& blob_holder, DEST& dest) { diff --git a/src/rgw/driver/sfs/sqlite/dbconn.cc b/src/rgw/driver/sfs/sqlite/dbconn.cc index 58274161fdf17a..3bbc5425c58a5b 100644 --- a/src/rgw/driver/sfs/sqlite/dbconn.cc +++ b/src/rgw/driver/sfs/sqlite/dbconn.cc @@ -205,12 +205,19 @@ dbapi::sqlite::database DBConn::get() { auto connection = storage_pool_new.at(this_thread); return dbapi::sqlite::database(connection); } catch (const std::out_of_range& ex) { - // using the same mutex as meanwhile code is being ported connections might - // be created for sqlite_orm code or sqlite_modern_cpp - std::unique_lock lock(storage_pool_mutex); - dbapi::sqlite::database db(getDBPath(cct)); - storage_pool_new.emplace(this_thread, db.connection()); - return db; + // call get_storage to open the connection the same way it was opened in + // the main thread. + get_storage(); + std::shared_lock lock(storage_pool_mutex); + if (storage_pool_new.find(this_thread) == storage_pool_new.end()) { + // something went really really wrong. + throw std::system_error( + ENOENT, std::system_category(), + "Could not find a valid SQLITE connection" + ); + } + auto connection = storage_pool_new.at(this_thread); + return dbapi::sqlite::database(connection); } } diff --git a/src/rgw/driver/sfs/sqlite/objects/object_definitions.h b/src/rgw/driver/sfs/sqlite/objects/object_definitions.h index 67839777df95fe..63331376f29d87 100644 --- a/src/rgw/driver/sfs/sqlite/objects/object_definitions.h +++ b/src/rgw/driver/sfs/sqlite/objects/object_definitions.h @@ -31,10 +31,10 @@ struct DBObject { DBObject() = default; - explicit DBObject(DBObjectQueryResult values) - : uuid(std::get<0>(values)), - bucket_id(std::get<1>(values)), - name(std::get<2>(values)) {} + explicit DBObject(DBObjectQueryResult&& values) + : uuid(std::move(std::get<0>(values))), + bucket_id(std::move(std::get<1>(values))), + name(std::move(std::get<2>(values))) {} }; } // namespace rgw::sal::sfs::sqlite diff --git a/src/rgw/driver/sfs/sqlite/sqlite_query_utils.h b/src/rgw/driver/sfs/sqlite/sqlite_query_utils.h index 949b29fac18e05..8098f9906e01b6 100644 --- a/src/rgw/driver/sfs/sqlite/sqlite_query_utils.h +++ b/src/rgw/driver/sfs/sqlite/sqlite_query_utils.h @@ -33,7 +33,7 @@ inline std::vector GetSQLiteObjects( auto rows = db << fmt::format("SELECT * FROM {};", table_name); std::vector ret; for (auto&& row : rows) { - ret.emplace_back(Target(row)); + ret.emplace_back(Target(std::move(row))); } return ret; } @@ -52,7 +52,7 @@ inline std::vector GetSQLiteObjectsWhere( << column_value; std::vector ret; for (auto&& row : rows) { - ret.emplace_back(Target(row)); + ret.emplace_back(Target(std::move(row))); } return ret; } @@ -64,13 +64,14 @@ inline std::optional GetSQLiteSingleObject( const std::string& key_name, const KeyType& key_value ) { auto rows = - db << fmt::format("SELECT * FROM {} WHERE {} = ?;", table_name, key_name) + db << fmt::format( + "SELECT * FROM {} WHERE {} = ? LIMIT 1;", table_name, key_name + ) << key_value; std::optional ret; - for (auto&& row : rows) { - ret = Target(row); - break; // looking for a single object, it should return 0 or 1 entries. - // TODO Return an error in there are more than 1 entry? + for (auto& row : rows) { + ret = Target(std::move(row)); + break; } return ret; } diff --git a/src/rgw/driver/sfs/sqlite/sqlite_users.cc b/src/rgw/driver/sfs/sqlite/sqlite_users.cc index baf6e4e2118b23..d8af835f005278 100644 --- a/src/rgw/driver/sfs/sqlite/sqlite_users.cc +++ b/src/rgw/driver/sfs/sqlite/sqlite_users.cc @@ -52,7 +52,7 @@ std::vector SQLiteUsers::get_user_ids() const { auto rows = db << R"sql(SELECT user_id FROM users;)sql"; std::vector ret; for (std::tuple row : rows) { - ret.emplace_back(std::get<0>(row)); + ret.emplace_back(std::move(std::get<0>(row))); } return ret; } @@ -120,7 +120,7 @@ std::optional SQLiteUsers::_get_user_id_by_access_key( WHERE access_key = ?;)sql" << key; for (std::tuple row : rows) { - ret = std::get<0>(row); + ret = std::move(std::get<0>(row)); // in case we have 2 keys that are equal in different users we return // the first one. // TODO Consider this an error? diff --git a/src/rgw/driver/sfs/sqlite/users/users_definitions.h b/src/rgw/driver/sfs/sqlite/users/users_definitions.h index aa5b99390a7978..d8cc74811e2bde 100644 --- a/src/rgw/driver/sfs/sqlite/users/users_definitions.h +++ b/src/rgw/driver/sfs/sqlite/users/users_definitions.h @@ -96,8 +96,8 @@ struct DBOPUserInfo { // sqlite_modern_cpp returns rows as a tuple. // This is a helper constructor for the case in which we want to get all the // columns and return a full object. - explicit DBOPUserInfo(DBUserQueryResult values) { - uinfo.user_id.id = std::get<0>(values); + explicit DBOPUserInfo(DBUserQueryResult&& values) { + uinfo.user_id.id = std::move(std::get<0>(values)); assign_optional_value(std::get<1>(values), uinfo.user_id.tenant); assign_optional_value(std::get<2>(values), uinfo.user_id.ns); assign_optional_value(std::get<3>(values), uinfo.display_name);