Some cleanups in Database_SQLite3

src/database/database-sqlite3.cpp

@@ -26,76 +26,69 @@ 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) {                             \
 		errorstream << (m) << ": "                      \
 			<< 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<s64 *>(data)[0];
-	s64 &prev_time = reinterpret_cast<s64 *>(data)[1];
-	s64 cur_time = porting::getTimeMs();
+	u64 &first_time = reinterpret_cast<u64*>(data)[0];
+	u64 &prev_time = reinterpret_cast<u64*>(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,16 +123,19 @@ 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");
 	}
 
 	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"

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);
 };