Some cleanups in Database_SQLite3

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.
+enum {
-#define BUSY_WARNING_TRESHOLD	250	// Print warning message after 250ms. Lag is increased.
+	BUSY_INFO_TRESHOLD    = 100,   // Print first informational message.
-#define BUSY_ERROR_TRESHOLD	1000	// Print error message after 1000ms. Significant lag.
+	BUSY_WARNING_TRESHOLD = 250,   // Print warning message. Significant lag.
-#define BUSY_FATAL_TRESHOLD	3000	// Allow SQLITE_BUSY to be returned, which will cause a minetest crash.
+	BUSY_FATAL_TRESHOLD   = 3000,  // Allow SQLITE_BUSY to be returned back to the caller.
-#define BUSY_ERROR_INTERVAL	10000	// Safety net: report again every 10 seconds
+	BUSY_ERROR_INTERVAL   = 10000, // Safety net: report again every 10 seconds
+};
 
-
+#define SQLRES(s, r, m) sqlite3_vrfy(s, m, r);
-#define SQLRES(s, r, m) \
-	if ((s) != (r)) { \
-		throw DatabaseException(std::string(m) + ": " +\
-				sqlite3_errmsg(m_database)); \
-	}
 #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),\
+	SQLOK(sqlite3_prepare_v2(m_database, query, -1, &m_stmt_##name, NULL), \
-		"Failed to prepare query '" query "'")
+		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), \
+#define FINALIZE_STATEMENT(name) \
-	"Failed to finalize " #statement)
+	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<s64 *>(data)[0];
+	u64 &first_time = reinterpret_cast<u64*>(data)[0];
-	s64 &prev_time = reinterpret_cast<s64 *>(data)[1];
+	u64 &prev_time = reinterpret_cast<u64*>(data)[1];
-	s64 cur_time = porting::getTimeMs();
+	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) {
+	const auto total_diff = cur_time - first_time; // time since first call
-		; // do nothing
+	const auto this_diff = prev_time - first_time; // time since last call
-	} else if (cur_time - first_time >= BUSY_INFO_TRESHOLD &&
+
-			prev_time - first_time < BUSY_INFO_TRESHOLD) {
+	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;
+			<< total_diff << " ms." << std::endl;
-	} else if (cur_time - first_time >= BUSY_WARNING_TRESHOLD &&
+	} else if (total_diff >= BUSY_WARNING_TRESHOLD &&
-			prev_time - first_time < BUSY_WARNING_TRESHOLD) {
+			this_diff < BUSY_WARNING_TRESHOLD) {
 		warningstream << "SQLite3 database has been locked for "
-			<< cur_time - first_time << " ms." << std::endl;
+			<< total_diff << " ms; this causes lag." << std::endl;
-	} else if (cur_time - first_time >= BUSY_ERROR_TRESHOLD &&
+	} else if (total_diff >= BUSY_FATAL_TRESHOLD &&
-			prev_time - first_time < BUSY_ERROR_TRESHOLD) {
+			this_diff < BUSY_FATAL_TRESHOLD) {
 		errorstream << "SQLite3 database has been locked for "
-			<< cur_time - first_time << " ms; this causes lag." << std::endl;
+			<< total_diff << " ms - giving up!" << std::endl;
-	} else if (cur_time - first_time >= BUSY_FATAL_TRESHOLD &&
+	} else if (total_diff / BUSY_ERROR_INTERVAL !=
-			prev_time - first_time < BUSY_FATAL_TRESHOLD) {
+			this_diff / BUSY_ERROR_INTERVAL) {
-		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) {
 		// 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,
+	auto flags = SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE;
-			SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE, NULL),
+#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(begin)
-	FINALIZE_STATEMENT(m_stmt_end)
+	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(read)
-	FINALIZE_STATEMENT(m_stmt_write)
+	FINALIZE_STATEMENT(write)
-	FINALIZE_STATEMENT(m_stmt_list)
+	FINALIZE_STATEMENT(list)
-	FINALIZE_STATEMENT(m_stmt_delete)
+	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)),
+	int64_to_sqlite(stmt, index, getBlockAsInteger(pos));
-		"Internal error: failed to bind query at " __FILE__ ":" TOSTRING(__LINE__));
 }
 
 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),
+	blob_to_sqlite(m_stmt_write, 2, data);
-		"Internal error: failed to bind query at " __FILE__ ":" TOSTRING(__LINE__));
 
 	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(player_load)
-	FINALIZE_STATEMENT(m_stmt_player_add)
+	FINALIZE_STATEMENT(player_add)
-	FINALIZE_STATEMENT(m_stmt_player_update)
+	FINALIZE_STATEMENT(player_update)
-	FINALIZE_STATEMENT(m_stmt_player_remove)
+	FINALIZE_STATEMENT(player_remove)
-	FINALIZE_STATEMENT(m_stmt_player_list)
+	FINALIZE_STATEMENT(player_list)
-	FINALIZE_STATEMENT(m_stmt_player_add_inventory)
+	FINALIZE_STATEMENT(player_add_inventory)
-	FINALIZE_STATEMENT(m_stmt_player_add_inventory_items)
+	FINALIZE_STATEMENT(player_add_inventory_items)
-	FINALIZE_STATEMENT(m_stmt_player_remove_inventory)
+	FINALIZE_STATEMENT(player_remove_inventory)
-	FINALIZE_STATEMENT(m_stmt_player_remove_inventory_items)
+	FINALIZE_STATEMENT(player_remove_inventory_items)
-	FINALIZE_STATEMENT(m_stmt_player_load_inventory)
+	FINALIZE_STATEMENT(player_load_inventory)
-	FINALIZE_STATEMENT(m_stmt_player_load_inventory_items)
+	FINALIZE_STATEMENT(player_load_inventory_items)
-	FINALIZE_STATEMENT(m_stmt_player_metadata_load)
+	FINALIZE_STATEMENT(player_metadata_load)
-	FINALIZE_STATEMENT(m_stmt_player_metadata_add)
+	FINALIZE_STATEMENT(player_metadata_add)
-	FINALIZE_STATEMENT(m_stmt_player_metadata_remove)
+	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` ("
@@ -401,7 +392,6 @@ void PlayerDatabaseSQLite3::initStatements()
 		"(`player`, `metadata`, `value`) VALUES (?, ?, ?)")
 	PREPARE_STATEMENT(player_metadata_remove, "DELETE FROM `player_metadata` "
 		"WHERE `player` = ?")
-	verbosestream << "ServerEnvironment: SQLite3 database opened (players)." << std::endl;
 }
 
 bool PlayerDatabaseSQLite3::playerDataExists(const std::string &name)
@@ -588,20 +578,20 @@ AuthDatabaseSQLite3::AuthDatabaseSQLite3(const std::string &savedir) :
 
 AuthDatabaseSQLite3::~AuthDatabaseSQLite3()
 {
-	FINALIZE_STATEMENT(m_stmt_read)
+	FINALIZE_STATEMENT(read)
-	FINALIZE_STATEMENT(m_stmt_write)
+	FINALIZE_STATEMENT(write)
-	FINALIZE_STATEMENT(m_stmt_create)
+	FINALIZE_STATEMENT(create)
-	FINALIZE_STATEMENT(m_stmt_delete)
+	FINALIZE_STATEMENT(delete)
-	FINALIZE_STATEMENT(m_stmt_list_names)
+	FINALIZE_STATEMENT(list_names)
-	FINALIZE_STATEMENT(m_stmt_read_privs)
+	FINALIZE_STATEMENT(read_privs)
-	FINALIZE_STATEMENT(m_stmt_write_privs)
+	FINALIZE_STATEMENT(write_privs)
-	FINALIZE_STATEMENT(m_stmt_delete_privs)
+	FINALIZE_STATEMENT(delete_privs)
-	FINALIZE_STATEMENT(m_stmt_last_insert_rowid)
+	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 +741,18 @@ ModStorageDatabaseSQLite3::ModStorageDatabaseSQLite3(const std::string &savedir)
 
 ModStorageDatabaseSQLite3::~ModStorageDatabaseSQLite3()
 {
-	FINALIZE_STATEMENT(m_stmt_remove_all)
+	FINALIZE_STATEMENT(remove_all)
-	FINALIZE_STATEMENT(m_stmt_remove)
+	FINALIZE_STATEMENT(remove)
-	FINALIZE_STATEMENT(m_stmt_set)
+	FINALIZE_STATEMENT(set)
-	FINALIZE_STATEMENT(m_stmt_has)
+	FINALIZE_STATEMENT(has)
-	FINALIZE_STATEMENT(m_stmt_get)
+	FINALIZE_STATEMENT(get)
-	FINALIZE_STATEMENT(m_stmt_get_keys)
+	FINALIZE_STATEMENT(get_keys)
-	FINALIZE_STATEMENT(m_stmt_get_all)
+	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"
@@ -825,8 +815,7 @@ bool ModStorageDatabaseSQLite3::getModEntry(const std::string &modname,
 	verifyDatabase();
 
 	str_to_sqlite(m_stmt_get, 1, modname);
-	SQLOK(sqlite3_bind_blob(m_stmt_get, 2, key.data(), key.size(), NULL),
+	blob_to_sqlite(m_stmt_get, 2, key);
-		"Internal error: failed to bind query at " __FILE__ ":" TOSTRING(__LINE__));
 	bool found = sqlite3_step(m_stmt_get) == SQLITE_ROW;
 	if (found) {
 		auto sv = sqlite_to_blob(m_stmt_get, 0);
@@ -845,8 +834,7 @@ bool ModStorageDatabaseSQLite3::hasModEntry(const std::string &modname,
 	verifyDatabase();
 
 	str_to_sqlite(m_stmt_has, 1, modname);
-	SQLOK(sqlite3_bind_blob(m_stmt_has, 2, key.data(), key.size(), NULL),
+	blob_to_sqlite(m_stmt_has, 2, key);
-		"Internal error: failed to bind query at " __FILE__ ":" TOSTRING(__LINE__));
 	bool found = sqlite3_step(m_stmt_has) == SQLITE_ROW;
 	if (found)
 		sqlite3_step(m_stmt_has);
@@ -862,10 +850,8 @@ bool ModStorageDatabaseSQLite3::setModEntry(const std::string &modname,
 	verifyDatabase();
 
 	str_to_sqlite(m_stmt_set, 1, modname);
-	SQLOK(sqlite3_bind_blob(m_stmt_set, 2, key.data(), key.size(), NULL),
+	blob_to_sqlite(m_stmt_set, 2, key);
-		"Internal error: failed to bind query at " __FILE__ ":" TOSTRING(__LINE__));
+	blob_to_sqlite(m_stmt_set, 3, value);
-	SQLOK(sqlite3_bind_blob(m_stmt_set, 3, value.data(), value.size(), NULL),
-		"Internal error: failed to bind query at " __FILE__ ":" TOSTRING(__LINE__));
 	SQLRES(sqlite3_step(m_stmt_set), SQLITE_DONE, "Failed to set mod entry")
 
 	sqlite3_reset(m_stmt_set);
@@ -879,8 +865,7 @@ bool ModStorageDatabaseSQLite3::removeModEntry(const std::string &modname,
 	verifyDatabase();
 
 	str_to_sqlite(m_stmt_remove, 1, modname);
-	SQLOK(sqlite3_bind_blob(m_stmt_remove, 2, key.data(), key.size(), NULL),
+	blob_to_sqlite(m_stmt_remove, 2, key);
-		"Internal error: failed to bind query at " __FILE__ ":" TOSTRING(__LINE__));
 	sqlite3_vrfy(sqlite3_step(m_stmt_remove), SQLITE_DONE);
 	int changes = sqlite3_changes(m_database);
 
@@ -906,6 +891,7 @@ void ModStorageDatabaseSQLite3::listMods(std::vector<std::string> *res)
 {
 	verifyDatabase();
 
+	// FIXME: please don't do this. this should be sqlite3_step like all others.
 	char *errmsg;
 	int status = sqlite3_exec(m_database,
 		"SELECT `modname` FROM `entries` GROUP BY `modname`;",

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,18 +23,25 @@ public:
 	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));
 	}
 
+	inline void blob_to_sqlite(sqlite3_stmt *s, int iCol, std::string_view str) const
+	{
+		sqlite3_vrfy(sqlite3_bind_blob(s, iCol, str.data(), str.size(), NULL));
+	}
+
 	inline void int_to_sqlite(sqlite3_stmt *s, int iCol, int val) const
 	{
 		sqlite3_vrfy(sqlite3_bind_int(s, iCol, val));
@@ -104,12 +112,14 @@ protected:
 				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 +129,27 @@ protected:
 		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 = "";
+	const std::string m_savedir;
-	std::string m_dbname = "";
+	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);
 };