From 7cd9442ac834f424a55efdc17577b116fb3deabc Mon Sep 17 00:00:00 2001 From: sfan5 Date: Sat, 8 Feb 2025 16:23:15 +0100 Subject: [PATCH] Some cleanups in Database_SQLite3 --- src/database/database-sqlite3.cpp | 177 ++++++++++++++---------------- src/database/database-sqlite3.h | 24 ++-- 2 files changed, 100 insertions(+), 101 deletions(-) diff --git a/src/database/database-sqlite3.cpp b/src/database/database-sqlite3.cpp index c3390fbe09d8c..e86e0c7466492 100644 --- a/src/database/database-sqlite3.cpp +++ b/src/database/database-sqlite3.cpp @@ -26,23 +26,19 @@ SQLite format specification: // When to print messages when the database is being held locked by another process // Note: I've seen occasional delays of over 250ms while running minetestmapper. -#define BUSY_INFO_TRESHOLD 100 // Print first informational message after 100ms. -#define BUSY_WARNING_TRESHOLD 250 // Print warning message after 250ms. Lag is increased. -#define BUSY_ERROR_TRESHOLD 1000 // Print error message after 1000ms. Significant lag. -#define BUSY_FATAL_TRESHOLD 3000 // Allow SQLITE_BUSY to be returned, which will cause a minetest crash. -#define BUSY_ERROR_INTERVAL 10000 // Safety net: report again every 10 seconds - +enum { + BUSY_INFO_TRESHOLD = 100, // Print first informational message. + BUSY_WARNING_TRESHOLD = 250, // Print warning message. Significant lag. + BUSY_FATAL_TRESHOLD = 3000, // Allow SQLITE_BUSY to be returned back to the caller. + BUSY_ERROR_INTERVAL = 10000, // Safety net: report again every 10 seconds +}; -#define SQLRES(s, r, m) \ - if ((s) != (r)) { \ - throw DatabaseException(std::string(m) + ": " +\ - sqlite3_errmsg(m_database)); \ - } +#define SQLRES(s, r, m) sqlite3_vrfy(s, m, r); #define SQLOK(s, m) SQLRES(s, SQLITE_OK, m) #define PREPARE_STATEMENT(name, query) \ - SQLOK(sqlite3_prepare_v2(m_database, query, -1, &m_stmt_##name, NULL),\ - "Failed to prepare query '" query "'") + SQLOK(sqlite3_prepare_v2(m_database, query, -1, &m_stmt_##name, NULL), \ + std::string("Failed to prepare query \"").append(query).append("\"")) #define SQLOK_ERRSTREAM(s, m) \ if ((s) != SQLITE_OK) { \ @@ -50,52 +46,49 @@ SQLite format specification: << sqlite3_errmsg(m_database) << std::endl; \ } -#define FINALIZE_STATEMENT(statement) SQLOK_ERRSTREAM(sqlite3_finalize(statement), \ - "Failed to finalize " #statement) +#define FINALIZE_STATEMENT(name) \ + sqlite3_finalize(m_stmt_##name); /* if this fails who cares */ \ + m_stmt_##name = nullptr; int Database_SQLite3::busyHandler(void *data, int count) { - s64 &first_time = reinterpret_cast(data)[0]; - s64 &prev_time = reinterpret_cast(data)[1]; - s64 cur_time = porting::getTimeMs(); + u64 &first_time = reinterpret_cast(data)[0]; + u64 &prev_time = reinterpret_cast(data)[1]; + u64 cur_time = porting::getTimeMs(); if (count == 0) { first_time = cur_time; prev_time = first_time; - } else { - while (cur_time < prev_time) - cur_time += s64(1)<<32; } - if (cur_time - first_time < BUSY_INFO_TRESHOLD) { - ; // do nothing - } else if (cur_time - first_time >= BUSY_INFO_TRESHOLD && - prev_time - first_time < BUSY_INFO_TRESHOLD) { + const auto total_diff = cur_time - first_time; // time since first call + const auto this_diff = prev_time - first_time; // time since last call + + if (total_diff < BUSY_INFO_TRESHOLD) { + // do nothing + } else if (total_diff >= BUSY_INFO_TRESHOLD && + this_diff < BUSY_INFO_TRESHOLD) { infostream << "SQLite3 database has been locked for " - << cur_time - first_time << " ms." << std::endl; - } else if (cur_time - first_time >= BUSY_WARNING_TRESHOLD && - prev_time - first_time < BUSY_WARNING_TRESHOLD) { + << total_diff << " ms." << std::endl; + } else if (total_diff >= BUSY_WARNING_TRESHOLD && + this_diff < BUSY_WARNING_TRESHOLD) { warningstream << "SQLite3 database has been locked for " - << cur_time - first_time << " ms." << std::endl; - } else if (cur_time - first_time >= BUSY_ERROR_TRESHOLD && - prev_time - first_time < BUSY_ERROR_TRESHOLD) { + << total_diff << " ms; this causes lag." << std::endl; + } else if (total_diff >= BUSY_FATAL_TRESHOLD && + this_diff < BUSY_FATAL_TRESHOLD) { errorstream << "SQLite3 database has been locked for " - << cur_time - first_time << " ms; this causes lag." << std::endl; - } else if (cur_time - first_time >= BUSY_FATAL_TRESHOLD && - prev_time - first_time < BUSY_FATAL_TRESHOLD) { - errorstream << "SQLite3 database has been locked for " - << cur_time - first_time << " ms - giving up!" << std::endl; - } else if ((cur_time - first_time) / BUSY_ERROR_INTERVAL != - (prev_time - first_time) / BUSY_ERROR_INTERVAL) { + << total_diff << " ms - giving up!" << std::endl; + } else if (total_diff / BUSY_ERROR_INTERVAL != + this_diff / BUSY_ERROR_INTERVAL) { // Safety net: keep reporting every BUSY_ERROR_INTERVAL errorstream << "SQLite3 database has been locked for " - << (cur_time - first_time) / 1000 << " seconds!" << std::endl; + << total_diff / 1000 << " seconds!" << std::endl; } prev_time = cur_time; // Make sqlite transaction fail if delay exceeds BUSY_FATAL_TRESHOLD - return cur_time - first_time < BUSY_FATAL_TRESHOLD; + return total_diff < BUSY_FATAL_TRESHOLD; } @@ -130,7 +123,7 @@ void Database_SQLite3::openDatabase() // Open the database connection if (!fs::CreateAllDirs(m_savedir)) { - infostream << "Database_SQLite3: Failed to create directory \"" + errorstream << "Database_SQLite3: Failed to create directory \"" << m_savedir << "\"" << std::endl; throw FileNotGoodException("Failed to create database " "save directory"); @@ -138,8 +131,11 @@ void Database_SQLite3::openDatabase() bool needs_create = !fs::PathExists(dbp); - SQLOK(sqlite3_open_v2(dbp.c_str(), &m_database, - SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE, NULL), + auto flags = SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE; +#ifdef SQLITE_OPEN_EXRESCODE + flags |= SQLITE_OPEN_EXRESCODE; +#endif + SQLOK(sqlite3_open_v2(dbp.c_str(), &m_database, flags, NULL), std::string("Failed to open SQLite3 database file ") + dbp); SQLOK(sqlite3_busy_handler(m_database, Database_SQLite3::busyHandler, @@ -152,9 +148,9 @@ void Database_SQLite3::openDatabase() std::string query_str = std::string("PRAGMA synchronous = ") + itos(g_settings->getU16("sqlite_synchronous")); SQLOK(sqlite3_exec(m_database, query_str.c_str(), NULL, NULL, NULL), - "Failed to modify sqlite3 synchronous mode"); + "Failed to set SQLite3 synchronous mode"); SQLOK(sqlite3_exec(m_database, "PRAGMA foreign_keys = ON", NULL, NULL, NULL), - "Failed to enable sqlite3 foreign key support"); + "Failed to enable SQLite3 foreign key support"); } void Database_SQLite3::verifyDatabase() @@ -173,8 +169,8 @@ void Database_SQLite3::verifyDatabase() Database_SQLite3::~Database_SQLite3() { - FINALIZE_STATEMENT(m_stmt_begin) - FINALIZE_STATEMENT(m_stmt_end) + FINALIZE_STATEMENT(begin) + FINALIZE_STATEMENT(end) SQLOK_ERRSTREAM(sqlite3_close(m_database), "Failed to close database"); } @@ -191,16 +187,16 @@ MapDatabaseSQLite3::MapDatabaseSQLite3(const std::string &savedir): MapDatabaseSQLite3::~MapDatabaseSQLite3() { - FINALIZE_STATEMENT(m_stmt_read) - FINALIZE_STATEMENT(m_stmt_write) - FINALIZE_STATEMENT(m_stmt_list) - FINALIZE_STATEMENT(m_stmt_delete) + FINALIZE_STATEMENT(read) + FINALIZE_STATEMENT(write) + FINALIZE_STATEMENT(list) + FINALIZE_STATEMENT(delete) } void MapDatabaseSQLite3::createDatabase() { - assert(m_database); // Pre-condition + assert(m_database); SQLOK(sqlite3_exec(m_database, "CREATE TABLE IF NOT EXISTS `blocks` (\n" @@ -217,14 +213,11 @@ void MapDatabaseSQLite3::initStatements() PREPARE_STATEMENT(write, "REPLACE INTO `blocks` (`pos`, `data`) VALUES (?, ?)"); PREPARE_STATEMENT(delete, "DELETE FROM `blocks` WHERE `pos` = ?"); PREPARE_STATEMENT(list, "SELECT `pos` FROM `blocks`"); - - verbosestream << "ServerMap: SQLite3 database opened." << std::endl; } inline void MapDatabaseSQLite3::bindPos(sqlite3_stmt *stmt, const v3s16 &pos, int index) { - SQLOK(sqlite3_bind_int64(stmt, index, getBlockAsInteger(pos)), - "Internal error: failed to bind query at " __FILE__ ":" TOSTRING(__LINE__)); + int64_to_sqlite(stmt, index, getBlockAsInteger(pos)); } bool MapDatabaseSQLite3::deleteBlock(const v3s16 &pos) @@ -237,7 +230,7 @@ bool MapDatabaseSQLite3::deleteBlock(const v3s16 &pos) sqlite3_reset(m_stmt_delete); if (!good) { - warningstream << "deleteBlock: Block failed to delete " + warningstream << "deleteBlock: Failed to delete block " << pos << ": " << sqlite3_errmsg(m_database) << std::endl; } return good; @@ -248,8 +241,7 @@ bool MapDatabaseSQLite3::saveBlock(const v3s16 &pos, std::string_view data) verifyDatabase(); bindPos(m_stmt_write, pos); - SQLOK(sqlite3_bind_blob(m_stmt_write, 2, data.data(), data.size(), NULL), - "Internal error: failed to bind query at " __FILE__ ":" TOSTRING(__LINE__)); + str_to_sqlite(m_stmt_write, 2, data); SQLRES(sqlite3_step(m_stmt_write), SQLITE_DONE, "Failed to save block") sqlite3_reset(m_stmt_write); @@ -271,7 +263,6 @@ void MapDatabaseSQLite3::loadBlock(const v3s16 &pos, std::string *block) auto data = sqlite_to_blob(m_stmt_read, 0); block->assign(data); - sqlite3_step(m_stmt_read); // We should never get more than 1 row, so ok to reset sqlite3_reset(m_stmt_read); } @@ -298,26 +289,26 @@ PlayerDatabaseSQLite3::PlayerDatabaseSQLite3(const std::string &savedir): PlayerDatabaseSQLite3::~PlayerDatabaseSQLite3() { - FINALIZE_STATEMENT(m_stmt_player_load) - FINALIZE_STATEMENT(m_stmt_player_add) - FINALIZE_STATEMENT(m_stmt_player_update) - FINALIZE_STATEMENT(m_stmt_player_remove) - FINALIZE_STATEMENT(m_stmt_player_list) - FINALIZE_STATEMENT(m_stmt_player_add_inventory) - FINALIZE_STATEMENT(m_stmt_player_add_inventory_items) - FINALIZE_STATEMENT(m_stmt_player_remove_inventory) - FINALIZE_STATEMENT(m_stmt_player_remove_inventory_items) - FINALIZE_STATEMENT(m_stmt_player_load_inventory) - FINALIZE_STATEMENT(m_stmt_player_load_inventory_items) - FINALIZE_STATEMENT(m_stmt_player_metadata_load) - FINALIZE_STATEMENT(m_stmt_player_metadata_add) - FINALIZE_STATEMENT(m_stmt_player_metadata_remove) + FINALIZE_STATEMENT(player_load) + FINALIZE_STATEMENT(player_add) + FINALIZE_STATEMENT(player_update) + FINALIZE_STATEMENT(player_remove) + FINALIZE_STATEMENT(player_list) + FINALIZE_STATEMENT(player_add_inventory) + FINALIZE_STATEMENT(player_add_inventory_items) + FINALIZE_STATEMENT(player_remove_inventory) + FINALIZE_STATEMENT(player_remove_inventory_items) + FINALIZE_STATEMENT(player_load_inventory) + FINALIZE_STATEMENT(player_load_inventory_items) + FINALIZE_STATEMENT(player_metadata_load) + FINALIZE_STATEMENT(player_metadata_add) + FINALIZE_STATEMENT(player_metadata_remove) }; void PlayerDatabaseSQLite3::createDatabase() { - assert(m_database); // Pre-condition + assert(m_database); SQLOK(sqlite3_exec(m_database, "CREATE TABLE IF NOT EXISTS `player` (" @@ -588,20 +579,20 @@ AuthDatabaseSQLite3::AuthDatabaseSQLite3(const std::string &savedir) : AuthDatabaseSQLite3::~AuthDatabaseSQLite3() { - FINALIZE_STATEMENT(m_stmt_read) - FINALIZE_STATEMENT(m_stmt_write) - FINALIZE_STATEMENT(m_stmt_create) - FINALIZE_STATEMENT(m_stmt_delete) - FINALIZE_STATEMENT(m_stmt_list_names) - FINALIZE_STATEMENT(m_stmt_read_privs) - FINALIZE_STATEMENT(m_stmt_write_privs) - FINALIZE_STATEMENT(m_stmt_delete_privs) - FINALIZE_STATEMENT(m_stmt_last_insert_rowid) + FINALIZE_STATEMENT(read) + FINALIZE_STATEMENT(write) + FINALIZE_STATEMENT(create) + FINALIZE_STATEMENT(delete) + FINALIZE_STATEMENT(list_names) + FINALIZE_STATEMENT(read_privs) + FINALIZE_STATEMENT(write_privs) + FINALIZE_STATEMENT(delete_privs) + FINALIZE_STATEMENT(last_insert_rowid) } void AuthDatabaseSQLite3::createDatabase() { - assert(m_database); // Pre-condition + assert(m_database); SQLOK(sqlite3_exec(m_database, "CREATE TABLE IF NOT EXISTS `auth` (" @@ -751,18 +742,18 @@ ModStorageDatabaseSQLite3::ModStorageDatabaseSQLite3(const std::string &savedir) ModStorageDatabaseSQLite3::~ModStorageDatabaseSQLite3() { - FINALIZE_STATEMENT(m_stmt_remove_all) - FINALIZE_STATEMENT(m_stmt_remove) - FINALIZE_STATEMENT(m_stmt_set) - FINALIZE_STATEMENT(m_stmt_has) - FINALIZE_STATEMENT(m_stmt_get) - FINALIZE_STATEMENT(m_stmt_get_keys) - FINALIZE_STATEMENT(m_stmt_get_all) + FINALIZE_STATEMENT(remove_all) + FINALIZE_STATEMENT(remove) + FINALIZE_STATEMENT(set) + FINALIZE_STATEMENT(has) + FINALIZE_STATEMENT(get) + FINALIZE_STATEMENT(get_keys) + FINALIZE_STATEMENT(get_all) } void ModStorageDatabaseSQLite3::createDatabase() { - assert(m_database); // Pre-condition + assert(m_database); SQLOK(sqlite3_exec(m_database, "CREATE TABLE IF NOT EXISTS `entries` (\n" diff --git a/src/database/database-sqlite3.h b/src/database/database-sqlite3.h index a89c0b1e7b425..4a26bc3456d8d 100644 --- a/src/database/database-sqlite3.h +++ b/src/database/database-sqlite3.h @@ -13,6 +13,7 @@ extern "C" { #include "sqlite3.h" } +// Template class for SQLite3 based data storage class Database_SQLite3 : public Database { public: @@ -22,13 +23,15 @@ class Database_SQLite3 : public Database void endSave(); bool initialized() const { return m_initialized; } + protected: Database_SQLite3(const std::string &savedir, const std::string &dbname); - // Open and initialize the database if needed + // Open and initialize the database if needed (not thread-safe) void verifyDatabase(); - // Convertors + /* Value conversion helpers */ + inline void str_to_sqlite(sqlite3_stmt *s, int iCol, std::string_view str) const { sqlite3_vrfy(sqlite3_bind_text(s, iCol, str.data(), str.size(), NULL)); @@ -104,12 +107,14 @@ class Database_SQLite3 : public Database sqlite_to_float(s, iCol + 2)); } - // Query verifiers helpers + // Helper for verifying result of sqlite3_step() and such inline void sqlite3_vrfy(int s, std::string_view m = "", int r = SQLITE_OK) const { if (s != r) { std::string msg(m); - msg.append(": ").append(sqlite3_errmsg(m_database)); + if (!msg.empty()) + msg.append(": "); + msg.append(sqlite3_errmsg(m_database)); throw DatabaseException(msg); } } @@ -119,24 +124,27 @@ class Database_SQLite3 : public Database sqlite3_vrfy(s, m, r); } - // Create the database structure + // Called after opening a fresh database file. Should create tables and indices. virtual void createDatabase() = 0; + + // Should prepare the necessary statements. virtual void initStatements() = 0; sqlite3 *m_database = nullptr; + private: // Open the database void openDatabase(); bool m_initialized = false; - std::string m_savedir = ""; - std::string m_dbname = ""; + const std::string m_savedir; + const std::string m_dbname; sqlite3_stmt *m_stmt_begin = nullptr; sqlite3_stmt *m_stmt_end = nullptr; - s64 m_busy_handler_data[2]; + u64 m_busy_handler_data[2]; static int busyHandler(void *data, int count); };