From 94f905a503b9d7d1aa3d9208cc86cf9d0582a673 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 28 Feb 2023 10:25:34 -0500 Subject: [PATCH 1/4] Fix SQLite DB schema migration code It now can safely run on bare databases, before any tables are created. Signed-off-by: Matthew Heon --- libpod/sqlite_state.go | 9 +++++---- libpod/sqlite_state_internal.go | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/libpod/sqlite_state.go b/libpod/sqlite_state.go index 8317fe4b7c..9082e736e5 100644 --- a/libpod/sqlite_state.go +++ b/libpod/sqlite_state.go @@ -77,15 +77,16 @@ func NewSqliteState(runtime *Runtime) (_ State, defErr error) { return nil, fmt.Errorf("setting full fsync mode in db: %w", err) } + // Migrate schema (if necessary) + if err := state.migrateSchemaIfNecessary(); err != nil { + return nil, err + } + // Set up tables if err := sqliteInitTables(state.conn); err != nil { return nil, fmt.Errorf("creating tables: %w", err) } - if err := state.migrateSchemaIfNecessary(); err != nil { - return nil, err - } - state.valid = true state.runtime = runtime diff --git a/libpod/sqlite_state_internal.go b/libpod/sqlite_state_internal.go index 1f62d346bd..184b96971e 100644 --- a/libpod/sqlite_state_internal.go +++ b/libpod/sqlite_state_internal.go @@ -16,6 +16,20 @@ import ( ) func (s *SQLiteState) migrateSchemaIfNecessary() (defErr error) { + // First, check if the DBConfig table exists + checkRow := s.conn.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) { + return nil + } + return fmt.Errorf("checking if DB config table exists: %w", err) + } + if check != 1 { + // Table does not exist, fresh database, no need to migrate. + return nil + } + row := s.conn.QueryRow("SELECT SchemaVersion FROM DBConfig;") var schemaVer int if err := row.Scan(&schemaVer); err != nil { @@ -24,6 +38,7 @@ func (s *SQLiteState) migrateSchemaIfNecessary() (defErr error) { // Schema was just created, so it has to be the latest. return nil } + return fmt.Errorf("scanning schema version from DB config: %w", err) } // If the schema version 0 or less, it's invalid From 9f0e0e8331638fc825dedd8b88c2ca63f742d96d Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 21 Mar 2023 09:57:56 -0400 Subject: [PATCH 2/4] Fix database locked errors with SQLite I was searching the SQLite docs for a fix, but apparently that was the wrong place; it's a common enough error with the Go frontend for SQLite that the fix is prominently listed in the API docs for go-sqlite3. Setting cache mode to 'shared' and using a maximum of 1 simultaneous open connection should fix. Performance implications of this are unclear, but cache=shared sounds like it will be a benefit, not a curse. [NO NEW TESTS NEEDED] This fixes a flake with concurrent DB access. Signed-off-by: Matthew Heon --- libpod/sqlite_state.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libpod/sqlite_state.go b/libpod/sqlite_state.go index 9082e736e5..7b9244913e 100644 --- a/libpod/sqlite_state.go +++ b/libpod/sqlite_state.go @@ -45,7 +45,7 @@ func NewSqliteState(runtime *Runtime) (_ State, defErr error) { return nil, fmt.Errorf("creating root directory: %w", err) } - conn, err := sql.Open("sqlite3", filepath.Join(basePath, "db.sql?_loc=auto")) + conn, err := sql.Open("sqlite3", filepath.Join(basePath, "db.sql?_loc=auto&cache=shared")) if err != nil { return nil, fmt.Errorf("initializing sqlite database: %w", err) } @@ -57,6 +57,9 @@ func NewSqliteState(runtime *Runtime) (_ State, defErr error) { } }() + // Necessary to avoid database locked errors. + conn.SetMaxOpenConns(1) + state.conn = conn if err := state.conn.Ping(); err != nil { From 0fbc325156d0c45d84e50be6b04e045363a0aebf Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 21 Mar 2023 11:09:59 +0100 Subject: [PATCH 3/4] sqlite: set connection attributes on open The symptoms in #17859 indicate that setting the PRAGMAs in individual EXECs outside of a transaction can lead to concurrency issues and failures when the DB is locked. Hence set all PRAGMAs when opening the connection. Move them into individual constants to improve documentation and readability. Further make transactions exclusive as #17859 also mentions an error that the DB is locked during a transaction. [NO NEW TESTS NEEDED] - existing tests cover the code. Fixes: #17859 Signed-off-by: Valentin Rothberg Signed-off-by: Matthew Heon --- libpod/sqlite_state.go | 40 +++++++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/libpod/sqlite_state.go b/libpod/sqlite_state.go index 7b9244913e..6f98668ec7 100644 --- a/libpod/sqlite_state.go +++ b/libpod/sqlite_state.go @@ -29,6 +29,30 @@ type SQLiteState struct { runtime *Runtime } +const ( + // Deal with timezone automatically. + sqliteOptionLocation = "_loc=auto" + // Set the journal mode (https://www.sqlite.org/pragma.html#pragma_journal_mode). + sqliteOptionJournal = "&_journal=WAL" + // Force WAL mode to fsync after each transaction (https://www.sqlite.org/pragma.html#pragma_synchronous). + sqliteOptionSynchronous = "&_sync=FULL" + // Allow foreign keys (https://www.sqlite.org/pragma.html#pragma_foreign_keys). + sqliteOptionForeignKeys = "&_foreign_keys=1" + // Enable cache sharing for threads within a process + sqliteOptionSharedCache = "&cache=shared" + // Make sure that transactions happen exclusively. + sqliteOptionTXLock = "&_txlock=exclusive" + + // Assembled sqlite options used when opening the database. + sqliteOptions = "db.sql?" + + sqliteOptionLocation + + sqliteOptionJournal + + sqliteOptionSynchronous + + sqliteOptionForeignKeys + + sqliteOptionSharedCache + + sqliteOptionTXLock +) + // NewSqliteState creates a new SQLite-backed state database. func NewSqliteState(runtime *Runtime) (_ State, defErr error) { state := new(SQLiteState) @@ -45,7 +69,7 @@ func NewSqliteState(runtime *Runtime) (_ State, defErr error) { return nil, fmt.Errorf("creating root directory: %w", err) } - conn, err := sql.Open("sqlite3", filepath.Join(basePath, "db.sql?_loc=auto&cache=shared")) + conn, err := sql.Open("sqlite3", filepath.Join(basePath, sqliteOptions)) if err != nil { return nil, fmt.Errorf("initializing sqlite database: %w", err) } @@ -66,20 +90,6 @@ func NewSqliteState(runtime *Runtime) (_ State, defErr error) { return nil, fmt.Errorf("cannot connect to database: %w", err) } - // Enable foreign keys constraints, which we use extensively in our - // tables. - if _, err := state.conn.Exec("PRAGMA foreign_keys = ON;"); err != nil { - return nil, fmt.Errorf("enabling foreign key support in database: %w", err) - } - // Enable WAL mode for performance - https://www.sqlite.org/wal.html - if _, err := state.conn.Exec("PRAGMA journal_mode = WAL;"); err != nil { - return nil, fmt.Errorf("switching journal to WAL mode: %w", err) - } - // Force WAL mode to fsync after every transaction, for reboot safety. - if _, err := state.conn.Exec("PRAGMA synchronous = FULL;"); err != nil { - return nil, fmt.Errorf("setting full fsync mode in db: %w", err) - } - // Migrate schema (if necessary) if err := state.migrateSchemaIfNecessary(); err != nil { return nil, err From 3925cd653b677fe2aab5f080faa64bb4c7714601 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 21 Mar 2023 14:20:34 -0400 Subject: [PATCH 4/4] Drop SQLite max connections The SQLite transaction lock Valentin found is (slightly) faster. So let's go with that. Signed-off-by: Matthew Heon --- libpod/sqlite_state.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/libpod/sqlite_state.go b/libpod/sqlite_state.go index 6f98668ec7..e1007dbc37 100644 --- a/libpod/sqlite_state.go +++ b/libpod/sqlite_state.go @@ -81,9 +81,6 @@ func NewSqliteState(runtime *Runtime) (_ State, defErr error) { } }() - // Necessary to avoid database locked errors. - conn.SetMaxOpenConns(1) - state.conn = conn if err := state.conn.Ping(); err != nil {