-
Notifications
You must be signed in to change notification settings - Fork 10
rgw/sfs: sqlite_modern_cpp blobs, users and objects changes #246
Changes from all commits
5f4ae90
0e21b59
1e15f9c
d63d69c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
#pragma once | ||
|
||
#include <sqlite3.h> | ||
|
||
namespace rgw::sal::sfs::dbapi { | ||
|
||
#include "sqlite_modern_cpp/hdr/sqlite_modern_cpp/type_wrapper.h" | ||
|
||
} // namespace rgw::sal::sfs::dbapi |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ | |
#include <sqlite3.h> | ||
|
||
#include <filesystem> | ||
#include <memory> | ||
#include <system_error> | ||
|
||
#include "common/dout.h" | ||
|
@@ -131,6 +132,14 @@ DBConn::DBConn(CephContext* _cct) | |
// storage->on_open() called from get_storage(), which has the exclusive | ||
// mutex. | ||
sqlite_conns.emplace_back(db); | ||
std::shared_ptr<sqlite3> db_connection = | ||
std::shared_ptr<sqlite3>(db, [=](sqlite3*) { | ||
// doing nothing for now... | ||
// this is just a workaround to reuse the connection opened from | ||
// sqlite_orm, the real owner of the connection is still sqlite_orm. | ||
// This won't be needed after code is fully ported to new sqlite lib | ||
}); | ||
storage_pool_new.emplace(std::this_thread::get_id(), db_connection); | ||
|
||
sqlite3_extended_result_codes(db, 1); | ||
sqlite3_busy_timeout(db, 10000); | ||
|
@@ -187,6 +196,31 @@ StorageRef DBConn::get_storage() { | |
} | ||
} | ||
|
||
dbapi::sqlite::database DBConn::get() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If a thread already called get_storage() this returns that connection (set in on_open) with the sqlite init we exect (busy timeout, error log, profiling settings) If that thread did not, this is a new connection with sqlite modern cpp default sqlite_open settings. We should avoid mixing sqlite connections with different initializations. Would it suffice to just call get_storage() in the catch, ignore the result and take storage_pool_new.at() which was set in on_open? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point (I was wondering about that too). I suspect that workaround you suggested should work. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, that's something I was afraid of. I will try that workaround. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed this to use |
||
std::thread::id this_thread = std::this_thread::get_id(); | ||
try { | ||
// using the same mutex as meanwhile code is being ported connections might | ||
// be created for sqlite_orm code or sqlite_modern_cpp | ||
std::shared_lock lock(storage_pool_mutex); | ||
auto connection = storage_pool_new.at(this_thread); | ||
return dbapi::sqlite::database(connection); | ||
} catch (const std::out_of_range& ex) { | ||
// 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); | ||
} | ||
} | ||
|
||
void DBConn::check_metadata_is_compatible() const { | ||
bool sync_error = false; | ||
std::string result_message; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -256,11 +256,15 @@ inline auto _make_storage(const std::string& path) { | |
using Storage = decltype(_make_storage("")); | ||
using StorageRef = Storage*; | ||
|
||
// TODO revisit this when code is fully ported to sqlite modern cpp | ||
using ConnectionNewLib = std::shared_ptr<sqlite3>; | ||
|
||
// TODO(https://github.com/aquarist-labs/s3gw/issues/788): Make | ||
// dbapi::sqlite::database the primary interface for sqlite3. | ||
class DBConn { | ||
private: | ||
std::unordered_map<std::thread::id, Storage> storage_pool; | ||
std::unordered_map<std::thread::id, ConnectionNewLib> storage_pool_new; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a general discussion note: It's a bit suboptimal that sqlite_modern_cpp want's a shared ptr. I'd rather have DBConn own all the sqlite connections with a unique_ptr. It's lifetime is already equal to sfs / RGW. The nice thing is that with sqlite_modern_cpp's std::shared_ptr constructor we can take full charge of the sqlite database initialization. |
||
std::vector<sqlite3*> sqlite_conns; | ||
const std::thread::id main_thread; | ||
mutable std::shared_mutex storage_pool_mutex; | ||
|
@@ -285,9 +289,7 @@ class DBConn { | |
return sqlite_conns; | ||
} | ||
|
||
dbapi::sqlite::database get() { | ||
return dbapi::sqlite::database(get_storage()->filename()); | ||
} | ||
dbapi::sqlite::database get(); | ||
|
||
static std::string getDBPath(CephContext* cct) { | ||
auto rgw_sfs_path = cct->_conf.get_val<std::string>("rgw_sfs_data_path"); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add also: RGWBucketInfo?
Saying this because I'm using encode/decode functions over this type but maybe not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we store the whole bucket info as a blob we can't then query over specific bucket columns. That's why we didn't store buckets that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right!