Improve error handling of map database creation

src/database/database-postgresql.cpp

@@ -97,7 +97,7 @@ void Database_PostgreSQL::ping()
 
 bool Database_PostgreSQL::initialized() const
 {
-	return (PQstatus(m_conn) == CONNECTION_OK);
+	return m_conn && PQstatus(m_conn) == CONNECTION_OK;
 }
 
 PGresult *Database_PostgreSQL::checkResults(PGresult *result, bool clear)

src/database/database-postgresql.h

@@ -9,20 +9,20 @@
 #include "database.h"
 #include "util/basic_macros.h"
 
-class Settings;
+// Template class for PostgreSQL based data storage
-
+class Database_PostgreSQL : public Database
-class Database_PostgreSQL: public Database
 {
 public:
 	Database_PostgreSQL(const std::string &connect_string, const char *type);
 	~Database_PostgreSQL();
 
-	void beginSave();
+	void beginSave() override;
-	void endSave();
+	void endSave() override;
 	void rollback();
 
-	bool initialized() const;
+	bool initialized() const override;
 
+	void verifyDatabase() override;
 
 protected:
 	// Conversion helpers
@@ -73,7 +73,6 @@ protected:
 	}
 
 	void createTableIfNotExists(const std::string &table_name, const std::string &definition);
-	void verifyDatabase();
 
 	// Database initialization
 	void connectToDatabase();
@@ -99,6 +98,12 @@ private:
 	int m_pgversion = 0;
 };
 
+// Not sure why why we have to do this. can't C++ figure it out on its own?
+#define PARENT_CLASS_FUNCS \
+	void beginSave() { Database_PostgreSQL::beginSave(); } \
+	void endSave() { Database_PostgreSQL::endSave(); } \
+	void verifyDatabase() { Database_PostgreSQL::verifyDatabase(); }
+
 class MapDatabasePostgreSQL : private Database_PostgreSQL, public MapDatabase
 {
 public:
@@ -110,8 +115,7 @@ public:
 	bool deleteBlock(const v3s16 &pos);
 	void listAllLoadableBlocks(std::vector<v3s16> &dst);
 
-	void beginSave() { Database_PostgreSQL::beginSave(); }
+	PARENT_CLASS_FUNCS
-	void endSave() { Database_PostgreSQL::endSave(); }
 
 protected:
 	virtual void createDatabase();
@@ -129,6 +133,8 @@ public:
 	bool removePlayer(const std::string &name);
 	void listPlayers(std::vector<std::string> &res);
 
+	PARENT_CLASS_FUNCS
+
 protected:
 	virtual void createDatabase();
 	virtual void initStatements();
@@ -143,8 +149,6 @@ public:
 	AuthDatabasePostgreSQL(const std::string &connect_string);
 	virtual ~AuthDatabasePostgreSQL() = default;
 
-	virtual void verifyDatabase() { Database_PostgreSQL::verifyDatabase(); }
-
 	virtual bool getAuth(const std::string &name, AuthEntry &res);
 	virtual bool saveAuth(const AuthEntry &authEntry);
 	virtual bool createAuth(AuthEntry &authEntry);
@@ -152,6 +156,8 @@ public:
 	virtual void listNames(std::vector<std::string> &res);
 	virtual void reload();
 
+	PARENT_CLASS_FUNCS
+
 protected:
 	virtual void createDatabase();
 	virtual void initStatements();
@@ -176,10 +182,11 @@ public:
 	bool removeModEntries(const std::string &modname);
 	void listMods(std::vector<std::string> *res);
 
-	void beginSave() { Database_PostgreSQL::beginSave(); }
+	PARENT_CLASS_FUNCS
-	void endSave() { Database_PostgreSQL::endSave(); }
 
 protected:
 	virtual void createDatabase();
 	virtual void initStatements();
 };
+
+#undef PARENT_CLASS_FUNCS

src/database/database-sqlite3.h

@@ -19,17 +19,17 @@ class Database_SQLite3 : public Database
 public:
 	virtual ~Database_SQLite3();
 
-	void beginSave();
+	void beginSave() override;
-	void endSave();
+	void endSave() override;
 
-	bool initialized() const { return m_initialized; }
+	bool initialized() const override { return m_initialized; }
+
+	/// @note not thread-safe
+	void verifyDatabase() override;
 
 protected:
 	Database_SQLite3(const std::string &savedir, const std::string &dbname);
 
-	// Open and initialize the database if needed (not thread-safe)
-	void verifyDatabase();
-
 	// Check if a specific table exists
 	bool checkTable(const char *table);
 
@@ -160,6 +160,12 @@ private:
 	static int busyHandler(void *data, int count);
 };
 
+// Not sure why why we have to do this. can't C++ figure it out on its own?
+#define PARENT_CLASS_FUNCS \
+	void beginSave() { Database_SQLite3::beginSave(); } \
+	void endSave() { Database_SQLite3::endSave(); } \
+	void verifyDatabase() { Database_SQLite3::verifyDatabase(); }
+
 class MapDatabaseSQLite3 : private Database_SQLite3, public MapDatabase
 {
 public:
@@ -171,8 +177,8 @@ public:
 	bool deleteBlock(const v3s16 &pos);
 	void listAllLoadableBlocks(std::vector<v3s16> &dst);
 
-	void beginSave() { Database_SQLite3::beginSave(); }
+	PARENT_CLASS_FUNCS
-	void endSave() { Database_SQLite3::endSave(); }
+
 protected:
 	virtual void createDatabase();
 	virtual void initStatements();
@@ -201,6 +207,8 @@ public:
 	bool removePlayer(const std::string &name);
 	void listPlayers(std::vector<std::string> &res);
 
+	PARENT_CLASS_FUNCS
+
 protected:
 	virtual void createDatabase();
 	virtual void initStatements();
@@ -238,6 +246,8 @@ public:
 	virtual void listNames(std::vector<std::string> &res);
 	virtual void reload();
 
+	PARENT_CLASS_FUNCS
+
 protected:
 	virtual void createDatabase();
 	virtual void initStatements();
@@ -273,8 +283,7 @@ public:
 	virtual bool removeModEntries(const std::string &modname);
 	virtual void listMods(std::vector<std::string> *res);
 
-	virtual void beginSave() { Database_SQLite3::beginSave(); }
+	PARENT_CLASS_FUNCS
-	virtual void endSave() { Database_SQLite3::endSave(); }
 
 protected:
 	virtual void createDatabase();
@@ -289,3 +298,5 @@ private:
 	sqlite3_stmt *m_stmt_remove = nullptr;
 	sqlite3_stmt *m_stmt_remove_all = nullptr;
 };
+
+#undef PARENT_CLASS_FUNCS

src/database/database.h

@@ -16,7 +16,12 @@ class Database
 public:
 	virtual void beginSave() = 0;
 	virtual void endSave() = 0;
+
+	/// @return true if database connection is open
 	virtual bool initialized() const { return true; }
+
+	/// Open and initialize the database if needed
+	virtual void verifyDatabase() {};
 };
 
 class MapDatabase : public Database

src/emerge.cpp

@@ -707,6 +707,8 @@ void *EmergeThread::run()
 			{
 				ScopeProfiler sp(g_profiler, "EmergeThread: load block - async (sum)");
 				MutexAutoLock dblock(m_db.mutex);
+				// Note: this can throw an exception, but there isn't really
+				// a good, safe way to handle it.
 				m_db.loadBlock(pos, databuf);
 			}
 			// actually load it, then decide again

src/server.cpp

@@ -473,8 +473,15 @@ void Server::init()
 	EnvAutoLock envlock(this);
 
 	// Create the Map (loads map_meta.txt, overriding configured mapgen params)
-	auto startup_server_map = std::make_unique<ServerMap>(m_path_world, this,
+	std::unique_ptr<ServerMap> startup_server_map;
-			m_emerge.get(), m_metrics_backend.get());
+	try {
+		startup_server_map = std::make_unique<ServerMap>(m_path_world, this,
+				m_emerge.get(), m_metrics_backend.get());
+	} catch (DatabaseException &e) {
+		throw ServerError(std::string(
+			"Failed to initialize the map database. The world may be "
+			"corrupted or in an unsupported format.\n") + e.what());
+	}
 
 	// Initialize scripting
 	infostream << "Server: Initializing Lua" << std::endl;

src/servermap.cpp

@@ -577,27 +577,34 @@ MapDatabase *ServerMap::createDatabase(
 	const std::string &savedir,
 	Settings &conf)
 {
+	MapDatabase *db = nullptr;
+
 	if (name == "sqlite3")
-		return new MapDatabaseSQLite3(savedir);
+		db = new MapDatabaseSQLite3(savedir);
 	if (name == "dummy")
-		return new Database_Dummy();
+		db = new Database_Dummy();
 	#if USE_LEVELDB
 	if (name == "leveldb")
-		return new Database_LevelDB(savedir);
+		db = new Database_LevelDB(savedir);
 	#endif
 	#if USE_REDIS
 	if (name == "redis")
-		return new Database_Redis(conf);
+		db = new Database_Redis(conf);
 	#endif
 	#if USE_POSTGRESQL
 	if (name == "postgresql") {
 		std::string connect_string;
 		conf.getNoEx("pgsql_connection", connect_string);
-		return new MapDatabasePostgreSQL(connect_string);
+		db = new MapDatabasePostgreSQL(connect_string);
 	}
 	#endif
 
-	throw BaseException(std::string("Database backend ") + name + " not supported.");
+	if (!db)
+		throw BaseException(std::string("Database backend ") + name + " not supported.");
+	// Do this to get feedback about errors asap
+	db->verifyDatabase();
+	assert(db->initialized());
+	return db;
 }
 
 void ServerMap::beginSave()