Run schema upgrades in one transaction and roll back stray failures

libmport/db.c

@@ -301,65 +301,65 @@ mport_generate_stub_schema(mportInstance *mport, sqlite3 *db)
 	return (MPORT_OK);
 }
 
-int
-mport_upgrade_master_schema(sqlite3 *db, int databaseVersion)
-{
-	if (databaseVersion == MPORT_MASTER_VERSION)
-		return MPORT_OK;
+/* Run one upgrade step; the caller owns the surrounding transaction. */
+#define UPGRADE_STEP(fn)                      \
+	do {                                  \
+		if (fn(db) != MPORT_OK)       \
+			RETURN_CURRENT_ERROR; \
+	} while (0)
 
+static int
+run_master_schema_upgrades(sqlite3 *db, int databaseVersion)
+{
 	switch (databaseVersion) {
 	case 0:
 	case 1:
-		mport_upgrade_master_schema_0to2(db);
-		mport_upgrade_master_schema_2to3(db);
-		mport_upgrade_master_schema_4to6(db);
-		mport_upgrade_master_schema_6to7(db);
-		mport_upgrade_master_schema_7to8(db);
-		mport_upgrade_master_schema_8to9(db);
-		mport_upgrade_master_schema_9to10(db);
-		mport_upgrade_master_schema_10to11(db);
-		mport_upgrade_master_schema_11to12(db);
-		mport_upgrade_master_schema_12to13(db);
-		mport_upgrade_master_schema_13to14(db);
-		mport_set_database_version(db);
+		UPGRADE_STEP(mport_upgrade_master_schema_0to2);
+		UPGRADE_STEP(mport_upgrade_master_schema_2to3);
+		UPGRADE_STEP(mport_upgrade_master_schema_4to6);
+		UPGRADE_STEP(mport_upgrade_master_schema_6to7);
+		UPGRADE_STEP(mport_upgrade_master_schema_7to8);
+		UPGRADE_STEP(mport_upgrade_master_schema_8to9);
+		UPGRADE_STEP(mport_upgrade_master_schema_9to10);
+		UPGRADE_STEP(mport_upgrade_master_schema_10to11);
+		UPGRADE_STEP(mport_upgrade_master_schema_11to12);
+		UPGRADE_STEP(mport_upgrade_master_schema_12to13);
+		UPGRADE_STEP(mport_upgrade_master_schema_13to14);
 		break;
 	case 2:
-		mport_upgrade_master_schema_2to3(db);
+		UPGRADE_STEP(mport_upgrade_master_schema_2to3);
 		/* falls through */
 	case 3:
-		mport_upgrade_master_schema_3to4(db);
+		UPGRADE_STEP(mport_upgrade_master_schema_3to4);
 		/* falls through */
 	case 4:
 		/* falls through */
 	case 5:
-		mport_upgrade_master_schema_4to6(db);
+		UPGRADE_STEP(mport_upgrade_master_schema_4to6);
 		/* falls through */
 	case 6:
+		UPGRADE_STEP(mport_upgrade_master_schema_6to7);
 		/* falls through */
-		mport_upgrade_master_schema_6to7(db);
 	case 7:
+		UPGRADE_STEP(mport_upgrade_master_schema_7to8);
 		/* falls through */
-		mport_upgrade_master_schema_7to8(db);
 	case 8:
+		UPGRADE_STEP(mport_upgrade_master_schema_8to9);
 		/* falls through */
-		mport_upgrade_master_schema_8to9(db);
 	case 9:
+		UPGRADE_STEP(mport_upgrade_master_schema_9to10);
 		/* falls through */
-		mport_upgrade_master_schema_9to10(db);
 	case 10:
+		UPGRADE_STEP(mport_upgrade_master_schema_10to11);
 		/* falls through */
-		mport_upgrade_master_schema_10to11(db);
 	case 11:
+		UPGRADE_STEP(mport_upgrade_master_schema_11to12);
 		/* falls through */
-		mport_upgrade_master_schema_11to12(db);
 	case 12:
+		UPGRADE_STEP(mport_upgrade_master_schema_12to13);
 		/* falls through */
-		mport_upgrade_master_schema_12to13(db);
 	case 13:
-		/* falls through */
-		mport_upgrade_master_schema_13to14(db);
-		mport_set_database_version(db);
-	case 14:
+		UPGRADE_STEP(mport_upgrade_master_schema_13to14);
 		break;
 	default:
 		RETURN_ERROR(MPORT_ERR_FATAL, "Invalid master database version");
@@ -368,6 +368,35 @@ mport_upgrade_master_schema(sqlite3 *db, int databaseVersion)
 	return (MPORT_OK);
 }
 
+/*
+ * Bring the master database up to MPORT_MASTER_VERSION.
+ *
+ * Every step and the version bump run in one transaction. SQLite DDL is
+ * transactional, so a failure part way (a locked registry, a step that no
+ * longer applies) leaves the schema and user_version exactly as they were
+ * instead of a half-upgraded database that the old version number makes
+ * mport try, and fail, to upgrade again on every start.
+ */
+int
+mport_upgrade_master_schema(sqlite3 *db, int databaseVersion)
+{
+	if (databaseVersion == MPORT_MASTER_VERSION)
+		return MPORT_OK;
+
+	if (mport_db_do(db, "BEGIN IMMEDIATE TRANSACTION") != MPORT_OK)
+		RETURN_CURRENT_ERROR;
+
+	if (run_master_schema_upgrades(db, databaseVersion) != MPORT_OK ||
+	    mport_set_database_version(db) != MPORT_OK ||
+	    mport_db_do(db, "COMMIT TRANSACTION") != MPORT_OK) {
+		/* sqlite3_exec directly so the rollback cannot clobber the error */
+		(void)sqlite3_exec(db, "ROLLBACK", NULL, NULL, NULL);
+		RETURN_CURRENT_ERROR;
+	}
+
+	return (MPORT_OK);
+}
+
 static int
 mport_upgrade_master_schema_0to2(sqlite3 *db)
 {
@@ -482,12 +511,11 @@ mport_upgrade_master_schema_12to13(sqlite3 *db)
 	    "CREATE TABLE IF NOT EXISTS conflicts (pkg text NOT NULL, conflict_pkg text NOT NULL, conflict_version text NOT NULL)");
 	RUN_SQL(db, "CREATE INDEX IF NOT EXISTS conflicts_pkg ON conflicts (pkg, conflict_pkg)");
 	RUN_SQL(db, "DROP INDEX IF EXISTS settings_name");
-	RUN_SQL(db, "BEGIN TRANSACTION;");
+	/* mport_upgrade_master_schema() runs the whole upgrade in one transaction */
 	RUN_SQL(db,
 	    "CREATE TABLE temp_settings AS SELECT MIN(rowid) as rowid, name, val FROM settings GROUP BY name;");
 	RUN_SQL(db, "DELETE FROM settings WHERE rowid NOT IN (SELECT rowid FROM temp_settings);");
 	RUN_SQL(db, "DROP TABLE temp_settings;");
-	RUN_SQL(db, "COMMIT;");
 	RUN_SQL(db, "CREATE UNIQUE INDEX IF NOT EXISTS settings_name_unique ON settings (name)");
 
 	return (MPORT_OK);

libmport/install_primative.c

@@ -295,18 +295,19 @@ purge_orphaned_rows(mportInstance *mport, const char *pkg_name)
 
 	for (i = 0; i < sizeof(tables) / sizeof(tables[0]); i++) {
 		if (mport_db_do(mport->db, "DELETE FROM %s WHERE pkg=%Q", tables[i], pkg_name) !=
-		    MPORT_OK) {
-			(void)mport_db_do(mport->db, "ROLLBACK");
-			RETURN_CURRENT_ERROR;
-		}
+		    MPORT_OK)
+			goto rollback;
 	}
 
-	if (mport_db_do(mport->db, "COMMIT TRANSACTION") != MPORT_OK) {
-		(void)mport_db_do(mport->db, "ROLLBACK");
-		RETURN_CURRENT_ERROR;
-	}
+	if (mport_db_do(mport->db, "COMMIT TRANSACTION") != MPORT_OK)
+		goto rollback;
 
 	return MPORT_OK;
+
+rollback:
+	/* sqlite3_exec directly so the rollback cannot clobber the error */
+	(void)sqlite3_exec(mport->db, "ROLLBACK", NULL, NULL, NULL);
+	RETURN_CURRENT_ERROR;
 }
 
 static int

libmport/merge_primative.c

@@ -225,29 +225,29 @@ build_stub_db(mportInstance *mport, sqlite3 **db, const char *tmpdir, const char
 			if (mport_db_do(
 				*db, "CREATE TABLE unsorted AS SELECT * FROM subbundle.packages") !=
 			    MPORT_OK)
-				RETURN_CURRENT_ERROR;
+				goto rollback;
 		} else {
 			if (mport_db_do(
 				*db, "INSERT INTO unsorted SELECT * FROM subbundle.packages") !=
 			    MPORT_OK)
-				RETURN_CURRENT_ERROR;
+				goto rollback;
 		}
 
 		if (mport_db_do(*db, "INSERT INTO assets SELECT * FROM subbundle.assets") !=
 		    MPORT_OK)
-			RETURN_CURRENT_ERROR;
+			goto rollback;
 		if (mport_db_do(*db, "INSERT INTO conflicts SELECT * FROM subbundle.conflicts") !=
 		    MPORT_OK)
-			RETURN_CURRENT_ERROR;
+			goto rollback;
 		if (mport_db_do(*db, "INSERT INTO depends SELECT * FROM subbundle.depends") !=
 		    MPORT_OK)
-			RETURN_CURRENT_ERROR;
+			goto rollback;
 
 		/* build our hashtable (pkgname => metadata) up */
 		if (mport_db_prepare(*db, &stmt, "SELECT pkg FROM subbundle.packages") !=
 		    MPORT_OK) {
 			sqlite3_finalize(stmt);
-			RETURN_CURRENT_ERROR;
+			goto rollback;
 		}
 
 		while (1) {
@@ -257,23 +257,29 @@ build_stub_db(mportInstance *mport, sqlite3 **db, const char *tmpdir, const char
 				name = sqlite3_column_text(stmt, 0);
 				if (insert_into_table(table, name, file) != MPORT_OK) {
 					sqlite3_finalize(stmt);
-					RETURN_CURRENT_ERROR;
+					goto rollback;
 				}
 			} else if (ret == SQLITE_DONE) {
 				break;
 			} else {
 				SET_ERROR(MPORT_ERR_FATAL, sqlite3_errmsg(*db));
 				sqlite3_finalize(stmt);
-				RETURN_CURRENT_ERROR;
+				goto rollback;
 			}
 		}
 
 		sqlite3_finalize(stmt);
 
 		if (mport_db_do(*db, "COMMIT TRANSACTION") != MPORT_OK)
-			RETURN_CURRENT_ERROR;
+			goto rollback;
 		if (mport_db_do(*db, "DETACH subbundle") != MPORT_OK)
 			RETURN_CURRENT_ERROR;
+		continue;
+
+	rollback:
+		/* sqlite3_exec directly so the rollback cannot clobber the error */
+		(void)sqlite3_exec(*db, "ROLLBACK", NULL, NULL, NULL);
+		RETURN_CURRENT_ERROR;
 	}
 
 	/* just have to sort the packages (going from unsorted to packages), no big deal... ;) */

libmport/verify.c

@@ -281,7 +281,7 @@ mport_recompute_checksums(mportInstance *mport, mportPackageMeta *pack)
 		if (ret != SQLITE_ROW) {
 			/* some error occured */
 			SET_ERROR(MPORT_ERR_FATAL, sqlite3_errmsg(mport->db));
-			mport_db_do(mport->db, "ROLLBACK");
+			(void)sqlite3_exec(mport->db, "ROLLBACK", NULL, NULL, NULL);
 			sqlite3_finalize(stmt);
 			sqlite3_finalize(update_stmt);
 			RETURN_CURRENT_ERROR;
@@ -376,8 +376,11 @@ mport_recompute_checksums(mportInstance *mport, mportPackageMeta *pack)
 	sqlite3_finalize(stmt);
 	sqlite3_finalize(update_stmt);
 
-	if (mport_db_do(mport->db, "COMMIT") != MPORT_OK)
+	if (mport_db_do(mport->db, "COMMIT") != MPORT_OK) {
+		/* sqlite3_exec directly so the rollback cannot clobber the error */
+		(void)sqlite3_exec(mport->db, "ROLLBACK", NULL, NULL, NULL);
 		RETURN_CURRENT_ERROR;
+	}
 
 	return (MPORT_OK);
 }

tests/mport_install_test.c

@@ -471,6 +471,68 @@ ATF_TC_CLEANUP(failed_delete_rolls_back, tc)
 	cleanup_test_root();
 }
 
+/*
+ * A schema upgrade that fails part way must leave the registry at the version
+ * it started from with none of the earlier steps applied, and must leave the
+ * connection usable so a retry can succeed.
+ */
+ATF_TC_WITH_CLEANUP(failed_schema_upgrade_rolls_back);
+ATF_TC_HEAD(failed_schema_upgrade_rolls_back, tc)
+{
+	atf_tc_set_md_var(tc, "descr", "a failed schema upgrade rolls back every step");
+}
+ATF_TC_BODY(failed_schema_upgrade_rolls_back, tc)
+{
+	mportInstance *mport;
+	int settings = -1;
+
+	(void)tc;
+
+	mport = create_test_instance();
+	ATF_REQUIRE_EQ(MPORT_MASTER_VERSION, mport_get_database_version(mport->db));
+
+	/* pretend the registry is at schema 11 and lacks the rows step 11to12 adds */
+	ATF_REQUIRE_EQ(MPORT_OK,
+	    mport_db_do(mport->db, "DELETE FROM settings WHERE name IN (%Q, %Q)",
+		MPORT_SETTING_HANDLE_RC_SCRIPTS, MPORT_SETTING_REPO_AUTOUPDATE));
+	ATF_REQUIRE_EQ(MPORT_OK, mport_db_do(mport->db, "PRAGMA user_version=11"));
+	ATF_REQUIRE_EQ(11, mport_get_database_version(mport->db));
+
+	/* step 12to13 creates this table, so its presence makes that step fail
+	 * after 11to12 has already run */
+	ATF_REQUIRE_EQ(MPORT_OK,
+	    mport_db_do(mport->db, "CREATE TABLE temp_settings (rowid int, name text, val text)"));
+
+	ATF_REQUIRE(mport_upgrade_master_schema(mport->db, 11) != MPORT_OK);
+
+	/* version and the rows added by the earlier step are both rolled back */
+	ATF_REQUIRE_EQ(11, mport_get_database_version(mport->db));
+	ATF_REQUIRE_EQ(MPORT_OK,
+	    mport_db_count(mport->db, &settings,
+		"SELECT COUNT(*) FROM settings WHERE name IN (%Q, %Q)",
+		MPORT_SETTING_HANDLE_RC_SCRIPTS, MPORT_SETTING_REPO_AUTOUPDATE));
+	ATF_REQUIRE_EQ(0, settings);
+
+	/* no transaction is left open, so a retry goes through */
+	ATF_REQUIRE_EQ(MPORT_OK, mport_db_do(mport->db, "DROP TABLE temp_settings"));
+	ATF_REQUIRE_MSG(
+	    mport_upgrade_master_schema(mport->db, 11) == MPORT_OK, "%s", mport_err_string());
+	ATF_REQUIRE_EQ(MPORT_MASTER_VERSION, mport_get_database_version(mport->db));
+	ATF_REQUIRE_EQ(MPORT_OK,
+	    mport_db_count(mport->db, &settings,
+		"SELECT COUNT(*) FROM settings WHERE name IN (%Q, %Q)",
+		MPORT_SETTING_HANDLE_RC_SCRIPTS, MPORT_SETTING_REPO_AUTOUPDATE));
+	ATF_REQUIRE_EQ(2, settings);
+
+	mport_instance_free(mport);
+}
+ATF_TC_CLEANUP(failed_schema_upgrade_rolls_back, tc)
+{
+	(void)tc;
+
+	cleanup_test_root();
+}
+
 /*
  * MidnightBSD does not expose arbitrary descriptors through /dev/fd/N.
  * Verify package installation can retain the verified descriptor instead of
@@ -522,6 +584,7 @@ ATF_TP_ADD_TCS(tp)
 	ATF_TP_ADD_TC(tp, force_reinstall_over_orphaned_rows);
 	ATF_TP_ADD_TC(tp, failed_install_registers_nothing);
 	ATF_TP_ADD_TC(tp, failed_delete_rolls_back);
+	ATF_TP_ADD_TC(tp, failed_schema_upgrade_rolls_back);
 
 	return atf_no_error();
 }