Skip to content
This repository has been archived by the owner on Jan 3, 2024. It is now read-only.

Rename database file to sfs.db #245

Merged
merged 3 commits into from
Nov 16, 2023
Merged

Rename database file to sfs.db #245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6aa58cf
rgw/sfs: Replace "s3gw.db" with constant
Nov 13, 2023
69c0648
rgw/sfs: Change database basename to 'sfs'
Nov 13, 2023
834fe55
rgw/sfs: Rename legacy database filename to new if exists on startup
Nov 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 64 additions & 1 deletion src/rgw/driver/sfs/sqlite/dbconn.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
*/
#include "dbconn.h"

#include <ceph_assert.h>
#include <sqlite3.h>

#include <filesystem>
Expand All @@ -30,7 +31,7 @@ namespace rgw::sal::sfs::sqlite {

static std::string get_temporary_db_path(CephContext* ctt) {
auto rgw_sfs_path = ctt->_conf.get_val<std::string>("rgw_sfs_data_path");
auto tmp_db_name = std::string(SCHEMA_DB_NAME) + "_tmp";
auto tmp_db_name = std::string(DB_FILENAME) + "_tmp";
auto db_path = std::filesystem::path(rgw_sfs_path) / std::string(tmp_db_name);
return db_path.string();
}
Expand Down Expand Up @@ -121,6 +122,7 @@ DBConn::DBConn(CephContext* _cct)
storage_pool_mutex(),
cct(_cct),
profile_enabled(_cct->_conf.get_val<bool>("rgw_sfs_sqlite_profile")) {
maybe_rename_database_file();
sqlite3_config(SQLITE_CONFIG_LOG, &sqlite_error_callback, cct);
// get_storage() relies on there already being an entry in the pool
// for the main thread (i.e. the thread that created the DBConn).
Expand Down Expand Up @@ -414,4 +416,65 @@ void DBConn::maybe_upgrade_metadata() {
}
}

void DBConn::maybe_rename_database_file() const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect we may also have to rename the -wal and -shm files. I think those can be left behind if the database is not properly closed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, if you killall -9 radosgw the -wal and -shm files will remain, then the next time you start it up, you'll see something like [SQLITE] (283) recovered 26 frames from WAL file /scratch/s3gw/qa/s3gw.db-wal in the log. So if we keep the migration code, we'd need to migrate those two as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it, the rename code is a bit naive. Perhaps too naive. Not only do we need to rename the extra files, there may also be temporary files (according to docs) that share the basename. If the database is still open renaming is also a big mistake.

A safer option would be the backup API - I think we should rather use that

if (!std::filesystem::exists(getLegacyDBPath(cct))) {
return;
}
if (std::filesystem::exists(getDBPath(cct))) {
return;
}
Comment on lines +420 to +425
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could have been a single if


lsubdout(cct, rgw_sfs, SFS_LOG_STARTUP)
<< fmt::format(
"Migrating legacy database file {} -> {}", getLegacyDBPath(cct),
getDBPath(cct)
)
<< dendl;

dbapi::sqlite::database src_db(getLegacyDBPath(cct));
dbapi::sqlite::database dst_db(getDBPath(cct));
auto state =
std::unique_ptr<sqlite3_backup, decltype(&sqlite3_backup_finish)>(
sqlite3_backup_init(
dst_db.connection().get(), "main", src_db.connection().get(),
"main"
),
sqlite3_backup_finish
);

if (!state) {
lsubdout(cct, rgw_sfs, SFS_LOG_ERROR)
<< fmt::format(
"Error opening legacy database file {} {}. Please migrate "
"s3gw.db to sfs.db manually",
getLegacyDBPath(cct), sqlite3_errmsg(dst_db.connection().get())
)
<< dendl;
ceph_abort_msg("sfs database file migration failed");
}

int rc = sqlite3_backup_step(state.get(), -1);
if (rc != SQLITE_DONE) {
lsubdout(cct, rgw_sfs, SFS_LOG_ERROR)
<< fmt::format(
"Error migrating legacy database file {} {}. Please migrate "
"s3gw.db to sfs.db manually",
getLegacyDBPath(cct), sqlite3_errmsg(dst_db.connection().get())
)
<< dendl;
ceph_abort_msg("sfs database file migration failed");
}

std::error_code ec; // ignore errors
fs::remove(getLegacyDBPath(cct), ec);
fs::remove(getLegacyDBPath(cct) + "-wal", ec);
fs::remove(getLegacyDBPath(cct) + "-shm", ec);

lsubdout(cct, rgw_sfs, SFS_LOG_STARTUP)
<< fmt::format(
"Done migrating legacy database. Continuing startup with {}",
getDBPath(cct)
)
<< dendl;
}
} // namespace rgw::sal::sfs::sqlite
14 changes: 12 additions & 2 deletions src/rgw/driver/sfs/sqlite/dbconn.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ constexpr int SFS_METADATA_VERSION = 4;
/// minimum required version to upgrade db.
constexpr int SFS_METADATA_MIN_VERSION = 4;

constexpr std::string_view SCHEMA_DB_NAME = "s3gw.db";
constexpr std::string_view LEGACY_DB_FILENAME = "s3gw.db";
constexpr std::string_view DB_FILENAME = "sfs.db";
constexpr std::string_view DB_WAL_FILENAME = "sfs.db-wal";

constexpr std::string_view USERS_TABLE = "users";
constexpr std::string_view BUCKETS_TABLE = "buckets";
Expand Down Expand Up @@ -292,12 +294,20 @@ class DBConn {
static std::string getDBPath(CephContext* cct) {
auto rgw_sfs_path = cct->_conf.get_val<std::string>("rgw_sfs_data_path");
auto db_path =
std::filesystem::path(rgw_sfs_path) / std::string(SCHEMA_DB_NAME);
std::filesystem::path(rgw_sfs_path) / std::string(DB_FILENAME);
return db_path.string();
}

static std::string getLegacyDBPath(CephContext* cct) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be perfectly honest, I really dislike that we have this function as camelCase when all other functions are snake case. More of an itch for me than anything else.

auto rgw_sfs_path = cct->_conf.get_val<std::string>("rgw_sfs_data_path");
auto db_path =
std::filesystem::path(rgw_sfs_path) / std::string(LEGACY_DB_FILENAME);
return db_path.string();
}

void check_metadata_is_compatible() const;
void maybe_upgrade_metadata();
void maybe_rename_database_file() const;
};

using DBConnRef = std::shared_ptr<DBConn>;
Expand Down
6 changes: 2 additions & 4 deletions src/test/rgw/sfs/test_rgw_sfs_gc.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,7 @@ class TestSFSGC : public ::testing::Test {
}

fs::path getDBFullPath(const std::string& base_dir) const {
auto db_full_name = "s3gw.db";
auto db_full_path = fs::path(base_dir) / db_full_name;
return db_full_path;
return fs::path(base_dir) / DB_FILENAME;
}

fs::path getDBFullPath() const { return getDBFullPath(getTestDir()); }
Expand All @@ -73,7 +71,7 @@ class TestSFSGC : public ::testing::Test {
[](const std::filesystem::path& path) {
return (
std::filesystem::is_regular_file(path) &&
!path.filename().string().starts_with("s3gw.db")
!path.filename().string().starts_with(DB_FILENAME)
);
}
);
Expand Down
4 changes: 1 addition & 3 deletions src/test/rgw/sfs/test_rgw_sfs_object_state_machine.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,7 @@ class TestSFSObjectStateMachine : public ::testing::Test {
}

fs::path getDBFullPath(const std::string& base_dir) const {
auto db_full_name = "s3gw.db";
auto db_full_path = fs::path(base_dir) / db_full_name;
return db_full_path;
return fs::path(base_dir) / sqlite::DB_FILENAME;
}

fs::path getDBFullPath() const { return getDBFullPath(getTestDir()); }
Expand Down
6 changes: 2 additions & 4 deletions src/test/rgw/sfs/test_rgw_sfs_sfs_bucket.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

/*
HINT
s3gw.db will create here: /tmp/rgw_sfs_tests
Creates sqlite files in /tmp/rgw_sfs_tests
*/

using namespace rgw::sal::sfs::sqlite;
Expand All @@ -43,9 +43,7 @@ class TestSFSBucket : public ::testing::Test {
}

fs::path getDBFullPath(const std::string& base_dir) const {
auto db_full_name = "s3gw.db";
auto db_full_path = fs::path(base_dir) / db_full_name;
return db_full_path;
return fs::path(base_dir) / DB_FILENAME;
}

fs::path getDBFullPath() const { return getDBFullPath(getTestDir()); }
Expand Down
6 changes: 2 additions & 4 deletions src/test/rgw/sfs/test_rgw_sfs_sfs_user.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

/*
HINT
s3gw.db will create here: /tmp/rgw_sfs_tests
Creates sqlite files in /tmp/rgw_sfs_tests
*/

using namespace rgw::sal::sfs::sqlite;
Expand Down Expand Up @@ -73,9 +73,7 @@ class TestSFSUser : public ::testing::Test {
}

fs::path getDBFullPath(const std::string& base_dir) const {
auto db_full_name = "s3gw.db";
auto db_full_path = fs::path(base_dir) / db_full_name;
return db_full_path;
return fs::path(base_dir) / DB_FILENAME;
}

fs::path getDBFullPath() const { return getDBFullPath(getTestDir()); }
Expand Down
4 changes: 1 addition & 3 deletions src/test/rgw/sfs/test_rgw_sfs_sqlite_buckets.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,7 @@ class TestSFSSQLiteBuckets : public ::testing::Test {
}

fs::path getDBFullPath(const std::string& base_dir) const {
auto db_full_name = "s3gw.db";
auto db_full_path = fs::path(base_dir) / db_full_name;
return db_full_path;
return fs::path(base_dir) / DB_FILENAME;
}

fs::path getDBFullPath() const { return getDBFullPath(getTestDir()); }
Expand Down
4 changes: 1 addition & 3 deletions src/test/rgw/sfs/test_rgw_sfs_sqlite_lifecycle.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,7 @@ class TestSFSSQLiteLifecycle : public ::testing::Test {
}

fs::path getDBFullPath(const std::string& base_dir) const {
auto db_full_name = "s3gw.db";
auto db_full_path = fs::path(base_dir) / db_full_name;
return db_full_path;
return fs::path(base_dir) / DB_FILENAME;
}

fs::path getDBFullPath() const { return getDBFullPath(getTestDir()); }
Expand Down
4 changes: 1 addition & 3 deletions src/test/rgw/sfs/test_rgw_sfs_sqlite_objects.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,7 @@ class TestSFSSQLiteObjects : public ::testing::Test {
}

fs::path getDBFullPath(const std::string& base_dir) const {
auto db_full_name = "s3gw.db";
auto db_full_path = fs::path(base_dir) / db_full_name;
return db_full_path;
return fs::path(base_dir) / DB_FILENAME;
}

fs::path getDBFullPath() const { return getDBFullPath(getTestDir()); }
Expand Down
4 changes: 1 addition & 3 deletions src/test/rgw/sfs/test_rgw_sfs_sqlite_users.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,7 @@ class TestSFSSQLiteUsers : public ::testing::Test {
}

fs::path getDBFullPath(const std::string& base_dir) const {
auto db_full_name = "s3gw.db";
auto db_full_path = fs::path(base_dir) / db_full_name;
return db_full_path;
return fs::path(base_dir) / DB_FILENAME;
}

fs::path getDBFullPath() const { return getDBFullPath(getTestDir()); }
Expand Down
4 changes: 1 addition & 3 deletions src/test/rgw/sfs/test_rgw_sfs_sqlite_versioned_objects.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,7 @@ class TestSFSSQLiteVersionedObjects : public ::testing::Test {
}

fs::path getDBFullPath(const std::string& base_dir) const {
auto db_full_name = "s3gw.db";
auto db_full_path = fs::path(base_dir) / db_full_name;
return db_full_path;
return fs::path(base_dir) / DB_FILENAME;
}

fs::path getDBFullPath() const { return getDBFullPath(getTestDir()); }
Expand Down
7 changes: 4 additions & 3 deletions src/test/rgw/sfs/test_rgw_sfs_wal_checkpoint.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#include <gtest/gtest.h>

#include "driver/sfs/sqlite/dbconn.h"
#include "rgw/rgw_sal_sfs.h"

using namespace rgw::sal::sfs;
Expand Down Expand Up @@ -78,7 +79,7 @@ class TestSFSWALCheckpoint : public ::testing::Test {
size_t num_threads, size_t num_objects
) {
std::atomic<std::uintmax_t> max_wal_size{0};
fs::path wal(test_dir / "s3gw.db-wal");
fs::path wal(test_dir / sqlite::DB_WAL_FILENAME);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we maybe define this as a function of DB_FILENAME, instead of having to keep a #define in the header solely for this test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would leak an sqlite implementation detail that a reader does not need to know about. I'd rather have this


std::vector<std::thread> threads;
for (size_t i = 0; i < num_threads; ++i) {
Expand Down Expand Up @@ -123,7 +124,7 @@ TEST_F(TestSFSWALCheckpoint, confirm_wal_explosion) {
// The fact that we have no size limit set means the WAL
// won't be truncated even when the last writer completes,
// so it should *still* be huge now.
EXPECT_EQ(fs::file_size(test_dir / "s3gw.db-wal"), max_wal_size);
EXPECT_EQ(fs::file_size(test_dir / sqlite::DB_WAL_FILENAME), max_wal_size);
}

// This test proves the WAL growth problem has been fixed
Expand All @@ -142,5 +143,5 @@ TEST_F(TestSFSWALCheckpoint, test_wal_checkpoint) {

// Once the writes are all done, the WAL should be finally
// truncated to something less than 16MB.
EXPECT_LT(fs::file_size(test_dir / "s3gw.db-wal"), SIZE_1MB * 16);
EXPECT_LT(fs::file_size(test_dir / sqlite::DB_WAL_FILENAME), SIZE_1MB * 16);
}
Loading