sqlite: move first read into a transaction

According to an old upstream issue [1]: "If the first statement after
BEGIN DEFERRED is a SELECT, then a read transaction is started.
Subsequent write statements will upgrade the transaction to a write
transaction if possible, or return SQLITE_BUSY."

So let's move the first SELECT under the same transaction as the table
initialization.

[NO NEW TESTS NEEDED] as it's a hard to cause race.

[1] mattn/go-sqlite3#274 (comment)

Fixes: containers#17859
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

libpod/sqlite_state.go

@@ -80,16 +80,32 @@ func NewSqliteState(runtime *Runtime) (_ State, defErr error) {
 
 	state.conn = conn
 
-	// Migrate schema (if necessary)
-	if err := state.migrateSchemaIfNecessary(); err != nil {
+	// Start with a transaction to avoid "database locked" errors.
+	// See https://github.com/mattn/go-sqlite3/issues/274#issuecomment-1429054597
+	tx, err := conn.Begin()
+	if err != nil {
+		return nil, fmt.Errorf("beginning transaction: %w", err)
+	}
+	defer func() {
+		if defErr != nil {
+			if err := tx.Rollback(); err != nil {
+				logrus.Errorf("Rolling back transaction to create tables: %v", err)
+			}
+		}
+	}()
+
+	if err := migrateSchemaIfNecessary(tx); err != nil {
 		return nil, err
 	}
 
-	// Set up tables
-	if err := sqliteInitTables(state.conn); err != nil {
+	if err := sqliteInitTables(tx); err != nil {
 		return nil, fmt.Errorf("creating tables: %w", err)
 	}
 
+	if err := tx.Commit(); err != nil {
+		return nil, fmt.Errorf("committing transaction: %w", err)
+	}
+
 	state.valid = true
 	state.runtime = runtime
 

libpod/sqlite_state_internal.go

@@ -15,9 +15,9 @@ import (
 	_ "github.com/mattn/go-sqlite3"
 )
 
-func (s *SQLiteState) migrateSchemaIfNecessary() (defErr error) {
+func migrateSchemaIfNecessary(tx *sql.Tx) error {
 	// First, check if the DBConfig table exists
-	checkRow := s.conn.QueryRow("SELECT 1 FROM sqlite_master WHERE type='table' AND name='DBConfig';")
+	checkRow := tx.QueryRow("SELECT 1 FROM sqlite_master WHERE type='table' AND name='DBConfig';")
 	var check int
 	if err := checkRow.Scan(&check); err != nil {
 		if errors.Is(err, sql.ErrNoRows) {
@@ -30,7 +30,7 @@ func (s *SQLiteState) migrateSchemaIfNecessary() (defErr error) {
 		return nil
 	}
 
-	row := s.conn.QueryRow("SELECT SchemaVersion FROM DBConfig;")
+	row := tx.QueryRow("SELECT SchemaVersion FROM DBConfig;")
 	var schemaVer int
 	if err := row.Scan(&schemaVer); err != nil {
 		if errors.Is(err, sql.ErrNoRows) {
@@ -60,7 +60,7 @@ func (s *SQLiteState) migrateSchemaIfNecessary() (defErr error) {
 }
 
 // Initialize all required tables for the SQLite state
-func sqliteInitTables(conn *sql.DB) (defErr error) {
+func sqliteInitTables(tx *sql.Tx) error {
 	// Technically we could split the "CREATE TABLE IF NOT EXISTS" and ");"
 	// bits off each command and add them in the for loop where we actually
 	// run the SQL, but that seems unnecessary.
@@ -186,28 +186,12 @@ func sqliteInitTables(conn *sql.DB) (defErr error) {
 		"VolumeState":          volumeState,
 	}
 
-	tx, err := conn.Begin()
-	if err != nil {
-		return fmt.Errorf("beginning transaction: %w", err)
-	}
-	defer func() {
-		if defErr != nil {
-			if err := tx.Rollback(); err != nil {
-				logrus.Errorf("Rolling back transaction to create tables: %v", err)
-			}
-		}
-	}()
-
 	for tblName, cmd := range tables {
 		if _, err := tx.Exec(cmd); err != nil {
 			return fmt.Errorf("creating table %s: %w", tblName, err)
 		}
 	}
 
-	if err := tx.Commit(); err != nil {
-		return fmt.Errorf("committing transaction: %w", err)
-	}
-
 	return nil
 }