From d63d69c0dc4454348c966e14597408f3d56dbe52 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 ++-- src/rgw/driver/sfs/sqlite/sqlite_objects.cc | 6 +-- .../driver/sfs/sqlite/sqlite_query_utils.h | 16 ++++--- src/rgw/driver/sfs/sqlite/sqlite_users.cc | 10 ++--- .../sfs/sqlite/users/users_definitions.h | 4 +- 8 files changed, 70 insertions(+), 65 deletions(-) diff --git a/src/rgw/driver/sfs/sqlite/bindings/blob.h b/src/rgw/driver/sfs/sqlite/bindings/blob.h index 7de28bb23abc7..abe0b938ce1f8 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 de0f906924380..b6fc1f2641906 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 58274161fdf17..3bbc5425c58a5 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 67839777df95f..63331376f29d8 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_objects.cc b/src/rgw/driver/sfs/sqlite/sqlite_objects.cc index 6a037f3d8ff30..75d937032bb5e 100644 --- a/src/rgw/driver/sfs/sqlite/sqlite_objects.cc +++ b/src/rgw/driver/sfs/sqlite/sqlite_objects.cc @@ -42,10 +42,10 @@ std::optional SQLiteObjects::get_object( << R"sql(SELECT * FROM objects WHERE bucket_id = ? AND name = ?;)sql" << bucket_id << object_name; std::optional ret_object; - for (auto&& row : rows) { + auto iter = rows.begin(); + if (iter != rows.end()) { + auto&& row = *iter; ret_object = DBObject(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? } return ret_object; } diff --git a/src/rgw/driver/sfs/sqlite/sqlite_query_utils.h b/src/rgw/driver/sfs/sqlite/sqlite_query_utils.h index 949b29fac18e0..7eb934fe8da55 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,15 @@ 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? + auto iter = rows.begin(); + if (iter != rows.end()) { + auto&& row = *iter; + ret = Target(std::move(row)); } return ret; } diff --git a/src/rgw/driver/sfs/sqlite/sqlite_users.cc b/src/rgw/driver/sfs/sqlite/sqlite_users.cc index baf6e4e2118b2..f7e9d6990bd98 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; } @@ -119,12 +119,10 @@ std::optional SQLiteUsers::_get_user_id_by_access_key( SELECT user_id FROM access_keys WHERE access_key = ?;)sql" << key; - for (std::tuple row : rows) { + auto iter = rows.begin(); + if (iter != rows.end()) { + std::tuple row = *iter; ret = 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? - break; } return ret; } diff --git a/src/rgw/driver/sfs/sqlite/users/users_definitions.h b/src/rgw/driver/sfs/sqlite/users/users_definitions.h index aa5b99390a797..d8cc74811e2bd 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);