Merge pull request #9647 from bhandras/sqldb-migration-base-version

sqldb: establish a base DB version even if it's not yet tracked

docs/release-notes/release-notes-0.19.0.md

@@ -455,6 +455,9 @@ the on going rate we'll permit.
   flag](https://github.com/lightningnetwork/lnd/pull/9606/) for future
   compatibility.
 
+* [Establish a base DB version even if it is not yet
+  tracked](https://github.com/lightningnetwork/lnd/pull/9647).
+
 ## Code Health
 
 * A code refactor that [moves all the graph related DB code out of the 

go.mod

@@ -207,6 +207,10 @@ replace github.com/gogo/protobuf => github.com/gogo/protobuf v1.3.2
 // allows us to specify that as an option.
 replace google.golang.org/protobuf => github.com/lightninglabs/protobuf-go-hex-display v1.30.0-hex-display
 
+// Temporary replace until https://github.com/lightningnetwork/lnd/pull/9647 is
+// merged.
+replace github.com/lightningnetwork/lnd/sqldb => ./sqldb
+
 // If you change this please also update docs/INSTALL.md and GO_VERSION in
 // Makefile (then run `make lint` to see where else it needs to be updated as
 // well).

go.sum

@@ -464,8 +464,6 @@ github.com/lightningnetwork/lnd/kvdb v1.4.12 h1:Y0WY5Tbjyjn6eCYh068qkWur5oFtioJl
 github.com/lightningnetwork/lnd/kvdb v1.4.12/go.mod h1:hx9buNcxsZpZwh8m1sjTQwy2SOeBoWWOZ3RnOQkMsxI=
 github.com/lightningnetwork/lnd/queue v1.1.1 h1:99ovBlpM9B0FRCGYJo6RSFDlt8/vOkQQZznVb18iNMI=
 github.com/lightningnetwork/lnd/queue v1.1.1/go.mod h1:7A6nC1Qrm32FHuhx/mi1cieAiBZo5O6l8IBIoQxvkz4=
-github.com/lightningnetwork/lnd/sqldb v1.0.7 h1:wQ4DdHY++uwxwth2CHL7s+duGqmMLaoIRBOQCa9HPTk=
-github.com/lightningnetwork/lnd/sqldb v1.0.7/go.mod h1:OG09zL/PHPaBJefp4HsPz2YLUJ+zIQHbpgCtLnOx8I4=
 github.com/lightningnetwork/lnd/ticker v1.1.1 h1:J/b6N2hibFtC7JLV77ULQp++QLtCwT6ijJlbdiZFbSM=
 github.com/lightningnetwork/lnd/ticker v1.1.1/go.mod h1:waPTRAAcwtu7Ji3+3k+u/xH5GHovTsCoSVpho0KDvdA=
 github.com/lightningnetwork/lnd/tlv v1.3.0 h1:exS/KCPEgpOgviIttfiXAPaUqw2rHQrnUOpP7HPBPiY=

sqldb/migrations.go

@@ -112,6 +112,15 @@ type MigrationExecutor interface {
 	// order. Migration details are stored in the global variable
 	// migrationConfig.
 	ExecuteMigrations(target MigrationTarget) error
+
+	// GetSchemaVersion returns the current schema version of the database.
+	GetSchemaVersion() (int, bool, error)
+
+	// SetSchemaVersion sets the schema version of the database.
+	//
+	// NOTE: This alters the internal database schema tracker. USE WITH
+	// CAUTION!!!
+	SetSchemaVersion(version int, dirty bool) error
 }
 
 var (
@@ -360,6 +369,46 @@ func ApplyMigrations(ctx context.Context, db *BaseDB,
 		}
 
 		currentVersion = int(version)
+	} else {
+		// Since we don't have a version tracked by our own table yet,
+		// we'll use the schema version reported by sqlc to determine
+		// the current version.
+		//
+		// NOTE: This is safe because the first in-code migration was
+		// introduced in version 7. This is only possible if the user
+		// has a schema version <= 4.
+		var dirty bool
+		currentVersion, dirty, err = migrator.GetSchemaVersion()
+		if err != nil {
+			return err
+		}
+
+		log.Infof("No database version found, using schema version %d "+
+			"(dirty=%v) as base version", currentVersion, dirty)
+	}
+
+	// Due to an a migration issue in v0.19.0-rc1 we may be at version 2 and
+	// have a dirty schema due to failing migration 3. If this is indeed the
+	// case, we need to reset the dirty flag to be able to apply the fixed
+	// migration.
+	// NOTE: this could be removed as soon as we drop v0.19.0-beta.
+	if version == 2 {
+		schemaVersion, dirty, err := migrator.GetSchemaVersion()
+		if err != nil {
+			return err
+		}
+
+		if schemaVersion == 3 && dirty {
+			log.Warnf("Schema version %d is dirty. This is "+
+				"likely a consequence of a failed migration "+
+				"in v0.19.0-rc1. Attempting to recover by "+
+				"resetting the dirty flag", schemaVersion)
+
+			err = migrator.SetSchemaVersion(4, false)
+			if err != nil {
+				return err
+			}
+		}
 	}
 
 	for _, migration := range migrations {

sqldb/migrations_test.go

@@ -452,3 +452,294 @@ func TestCustomMigration(t *testing.T) {
 		})
 	}
 }
+
+// TestSchemaMigrationIdempotency tests that the our schema migrations are
+// idempotent. This means that we can apply the migrations multiple times and
+// the schema version will always be the same.
+func TestSchemaMigrationIdempotency(t *testing.T) {
+	dropMigrationTrackerEntries := func(t *testing.T, db *BaseDB) {
+		_, err := db.Exec("DELETE FROM migration_tracker;")
+		require.NoError(t, err)
+	}
+
+	lastMigration := migrationConfig[len(migrationConfig)-1]
+
+	t.Run("SQLite", func(t *testing.T) {
+		// First instantiate the database and run the migrations
+		// including the custom migrations.
+		t.Logf("Creating new SQLite DB for testing migrations")
+
+		dbFileName := filepath.Join(t.TempDir(), "tmp.db")
+		var (
+			db  *SqliteStore
+			err error
+		)
+
+		// Run the migration 3 times to test that the migrations
+		// are idempotent.
+		for i := 0; i < 3; i++ {
+			db, err = NewSqliteStore(&SqliteConfig{
+				SkipMigrations: false,
+			}, dbFileName)
+			require.NoError(t, err)
+
+			dbToCleanup := db.DB
+			t.Cleanup(func() {
+				require.NoError(
+					t, dbToCleanup.Close(),
+				)
+			})
+
+			ctxb := context.Background()
+			require.NoError(
+				t, db.ApplyAllMigrations(ctxb, GetMigrations()),
+			)
+
+			version, dirty, err := db.GetSchemaVersion()
+			require.NoError(t, err)
+
+			// Now reset the schema version to 0 and make sure that
+			// we can apply the migrations again.
+			require.Equal(t, lastMigration.SchemaVersion, version)
+			require.False(t, dirty)
+
+			require.NoError(
+				t, db.SetSchemaVersion(
+					database.NilVersion, false,
+				),
+			)
+			dropMigrationTrackerEntries(t, db.BaseDB)
+
+			// Make sure that we reset the schema version.
+			version, dirty, err = db.GetSchemaVersion()
+			require.NoError(t, err)
+			require.Equal(t, -1, version)
+			require.False(t, dirty)
+		}
+	})
+
+	t.Run("Postgres", func(t *testing.T) {
+		// First create a temporary Postgres database to run
+		// the migrations on.
+		fixture := NewTestPgFixture(
+			t, DefaultPostgresFixtureLifetime,
+		)
+		t.Cleanup(func() {
+			fixture.TearDown(t)
+		})
+
+		dbName := randomDBName(t)
+
+		// Next instantiate the database and run the migrations
+		// including the custom migrations.
+		t.Logf("Creating new Postgres DB '%s' for testing "+
+			"migrations", dbName)
+
+		_, err := fixture.db.ExecContext(
+			context.Background(), "CREATE DATABASE "+dbName,
+		)
+		require.NoError(t, err)
+
+		cfg := fixture.GetConfig(dbName)
+		var db *PostgresStore
+
+		// Run the migration 3 times to test that the migrations
+		// are idempotent.
+		for i := 0; i < 3; i++ {
+			cfg.SkipMigrations = false
+			db, err = NewPostgresStore(cfg)
+			require.NoError(t, err)
+
+			ctxb := context.Background()
+			require.NoError(
+				t, db.ApplyAllMigrations(ctxb, GetMigrations()),
+			)
+
+			version, dirty, err := db.GetSchemaVersion()
+			require.NoError(t, err)
+
+			// Now reset the schema version to 0 and make sure that
+			// we can apply the migrations again.
+			require.Equal(t, lastMigration.SchemaVersion, version)
+			require.False(t, dirty)
+
+			require.NoError(
+				t, db.SetSchemaVersion(
+					database.NilVersion, false,
+				),
+			)
+			dropMigrationTrackerEntries(t, db.BaseDB)
+
+			// Make sure that we reset the schema version.
+			version, dirty, err = db.GetSchemaVersion()
+			require.NoError(t, err)
+			require.Equal(t, -1, version)
+			require.False(t, dirty)
+		}
+	})
+}
+
+// TestMigrationBug19RC1 tests a bug that was present in the migration code
+// at the v0.19.0-rc1 release.
+// The bug was fixed in: https://github.com/lightningnetwork/lnd/pull/9647
+// NOTE: This test may be removed once the final version of 0.19.0 is released.
+func TestMigrationSucceedsAfterDirtyStateMigrationFailure19RC1(t *testing.T) {
+	// setMigrationTrackerVersion is a helper function that
+	// updates the migration tracker table to a specific version that
+	// simulates the conditions of the bug.
+	setMigrationTrackerVersion := func(t *testing.T, db *BaseDB) {
+		_, err := db.Exec(`
+	        DELETE FROM migration_tracker;
+		INSERT INTO migration_tracker (version, migration_time)
+			VALUES (2, CURRENT_TIMESTAMP);
+	        `)
+		require.NoError(t, err)
+	}
+
+	const (
+		maxSchemaVersionBefore19RC1 = 4
+		failingSchemaVersion        = 3
+	)
+
+	ctxb := context.Background()
+	migrations := GetMigrations()
+	migrations = migrations[:maxSchemaVersionBefore19RC1]
+	lastMigration := migrations[len(migrations)-1]
+
+	// Make sure that the last migration is the one we expect.
+	require.Equal(
+		t, maxSchemaVersionBefore19RC1, lastMigration.SchemaVersion,
+	)
+
+	t.Run("SQLite", func(t *testing.T) {
+		// First instantiate the database and run the migrations
+		// including the custom migrations.
+		t.Logf("Creating new SQLite DB for testing migrations")
+
+		dbFileName := filepath.Join(t.TempDir(), "tmp.db")
+		var (
+			db  *SqliteStore
+			err error
+		)
+
+		db, err = NewSqliteStore(&SqliteConfig{
+			SkipMigrations: false,
+		}, dbFileName)
+		require.NoError(t, err)
+
+		dbToCleanup := db.DB
+		t.Cleanup(func() {
+			require.NoError(t, dbToCleanup.Close())
+		})
+
+		require.NoError(t, db.ApplyAllMigrations(ctxb, migrations))
+
+		version, dirty, err := db.GetSchemaVersion()
+		require.NoError(t, err)
+
+		// Now reset the schema version to 0 and make sure that
+		// we can apply the migrations again.
+		require.Equal(t, lastMigration.SchemaVersion, version)
+		require.False(t, dirty)
+
+		// Set the schema version to the failing version and
+		// make make the version dirty which essentially tells
+		// golang-migrate that the migration failed.
+		require.NoError(
+			t, db.SetSchemaVersion(failingSchemaVersion, true),
+		)
+
+		// Set the migration tracker to the failing version.
+		setMigrationTrackerVersion(t, db.BaseDB)
+
+		// Close the DB so we can reopen it and apply the
+		// migrations again.
+		db.DB.Close()
+
+		db, err = NewSqliteStore(&SqliteConfig{
+			SkipMigrations: false,
+		}, dbFileName)
+		require.NoError(t, err)
+
+		dbToCleanup2 := db.DB
+		t.Cleanup(func() {
+			require.NoError(t, dbToCleanup2.Close())
+		})
+
+		require.NoError(t, db.ApplyAllMigrations(ctxb, migrations))
+
+		version, dirty, err = db.GetSchemaVersion()
+		require.NoError(t, err)
+		require.Equal(t, lastMigration.SchemaVersion, version)
+		require.False(t, dirty)
+	})
+
+	t.Run("Postgres", func(t *testing.T) {
+		// First create a temporary Postgres database to run
+		// the migrations on.
+		fixture := NewTestPgFixture(
+			t, DefaultPostgresFixtureLifetime,
+		)
+		t.Cleanup(func() {
+			fixture.TearDown(t)
+		})
+
+		dbName := randomDBName(t)
+
+		// Next instantiate the database and run the migrations
+		// including the custom migrations.
+		t.Logf("Creating new Postgres DB '%s' for testing "+
+			"migrations", dbName)
+
+		_, err := fixture.db.ExecContext(
+			context.Background(), "CREATE DATABASE "+dbName,
+		)
+		require.NoError(t, err)
+
+		cfg := fixture.GetConfig(dbName)
+		var db *PostgresStore
+
+		cfg.SkipMigrations = false
+		db, err = NewPostgresStore(cfg)
+		require.NoError(t, err)
+
+		require.NoError(t, db.ApplyAllMigrations(ctxb, migrations))
+
+		version, dirty, err := db.GetSchemaVersion()
+		require.NoError(t, err)
+
+		// Now reset the schema version to 0 and make sure that
+		// we can apply the migrations again.
+		require.Equal(t, lastMigration.SchemaVersion, version)
+		require.False(t, dirty)
+
+		// Set the schema version to the failing version and
+		// make make the version dirty which essentially tells
+		// golang-migrate that the migration failed.
+		require.NoError(
+			t, db.SetSchemaVersion(failingSchemaVersion, true),
+		)
+
+		// Set the migration tracker to the failing version.
+		setMigrationTrackerVersion(t, db.BaseDB)
+
+		// Close the DB so we can reopen it and apply the
+		// migrations again.
+		db.DB.Close()
+
+		db, err = NewPostgresStore(cfg)
+		require.NoError(t, err)
+
+		dbToCleanup2 := db.DB
+		t.Cleanup(func() {
+			require.NoError(t, dbToCleanup2.Close())
+		})
+
+		require.NoError(t, db.ApplyAllMigrations(ctxb, migrations))
+
+		version, dirty, err = db.GetSchemaVersion()
+		require.NoError(t, err)
+		require.Equal(t, lastMigration.SchemaVersion, version)
+		require.False(t, dirty)
+	})
+}

sqldb/postgres.go

@@ -158,6 +158,10 @@ func (s *PostgresStore) ApplyAllMigrations(ctx context.Context,
 	return ApplyMigrations(ctx, s.BaseDB, s, migrations)
 }
 
+func errPostgresMigration(err error) error {
+	return fmt.Errorf("error creating postgres migration: %w", err)
+}
+
 // ExecuteMigrations runs migrations for the Postgres database, depending on the
 // target given, either all migrations or up to a given version.
 func (s *PostgresStore) ExecuteMigrations(target MigrationTarget) error {
@@ -168,7 +172,7 @@ func (s *PostgresStore) ExecuteMigrations(target MigrationTarget) error {
 
 	driver, err := pgx_migrate.WithInstance(s.DB, &pgx_migrate.Config{})
 	if err != nil {
-		return fmt.Errorf("error creating postgres migration: %w", err)
+		return errPostgresMigration(err)
 	}
 
 	// Populate the database with our set of schemas based on our embedded
@@ -178,3 +182,31 @@ func (s *PostgresStore) ExecuteMigrations(target MigrationTarget) error {
 		postgresFS, driver, "sqlc/migrations", dbName, target,
 	)
 }
+
+// GetSchemaVersion returns the current schema version of the Postgres database.
+func (s *PostgresStore) GetSchemaVersion() (int, bool, error) {
+	driver, err := pgx_migrate.WithInstance(s.DB, &pgx_migrate.Config{})
+	if err != nil {
+		return 0, false, errPostgresMigration(err)
+
+	}
+
+	version, dirty, err := driver.Version()
+	if err != nil {
+		return 0, false, err
+	}
+
+	return version, dirty, nil
+}
+
+// SetSchemaVersion sets the schema version of the Postgres database.
+//
+// NOTE: This alters the internal database schema tracker. USE WITH CAUTION!!!
+func (s *PostgresStore) SetSchemaVersion(version int, dirty bool) error {
+	driver, err := pgx_migrate.WithInstance(s.DB, &pgx_migrate.Config{})
+	if err != nil {
+		return errPostgresMigration(err)
+	}
+
+	return driver.SetVersion(version, dirty)
+}

sqldb/sqlc/migrations/000001_invoices.up.sql

@@ -1,12 +1,14 @@
 -- sequences contains all sequences used for invoices.
-CREATE TABLE invoice_sequences (
+CREATE TABLE IF NOT EXISTS invoice_sequences (
     name TEXT PRIMARY KEY,
     current_value BIGINT NOT NULL
 );
 
 -- Initialize a sequence for the settled index used to track invoice settlement
 -- to remain compatible with the legacy channeldb implementation.
-INSERT INTO invoice_sequences(name, current_value) VALUES ('settle_index', 0);
+INSERT INTO invoice_sequences (name, current_value) 
+VALUES ('settle_index', 0) 
+    ON CONFLICT (name) DO NOTHING;
 
 -- invoices table contains all the information shared by all the invoice types. 
 CREATE TABLE IF NOT EXISTS invoices (

sqldb/sqlc/migrations/000003_invoice_events.up.sql

@@ -5,26 +5,31 @@ CREATE TABLE IF NOT EXISTS invoice_event_types(
     description TEXT NOT NULL
 );
 
+
 -- invoice_event_types defines the different types of invoice events.
-INSERT INTO invoice_event_types (id, description) 
-VALUES 
+-- Insert events explicitly, checking their existence first.
+INSERT INTO invoice_event_types (id, description)
+SELECT id, description FROM (
     -- invoice_created is the event type used when an invoice is created.
-    (0, 'invoice_created'), 
+    SELECT 0 AS id, 'invoice_created' AS description UNION ALL
     -- invoice_canceled is the event type used when an invoice is canceled.
-    (1, 'invoice_canceled'), 
+    SELECT 1, 'invoice_canceled' UNION ALL
     -- invoice_settled is the event type used when an invoice is settled.
-    (2, 'invoice_settled'),
+    SELECT 2, 'invoice_settled' UNION ALL
     -- setid_created is the event type used when an AMP sub invoice
     -- corresponding to the set_id is created.
-    (3, 'setid_created'),
+    SELECT 3, 'setid_created' UNION ALL
     -- setid_canceled is the event type used when an AMP sub invoice
     -- corresponding to the set_id is canceled.
-    (4, 'setid_canceled'),
-    -- setid_settled is the event type used when an AMP sub invoice 
+    SELECT 4, 'setid_canceled' UNION ALL
+    -- setid_settled is the event type used when an AMP sub invoice
     -- corresponding to the set_id is settled.
-    (5, 'setid_settled');
-
-
+    SELECT 5, 'setid_settled'
+) AS new_values
+WHERE NOT EXISTS (
+    SELECT 1 FROM invoice_event_types
+        WHERE invoice_event_types.id = new_values.id
+);
 -- invoice_events stores all major events related to the node's invoices and
 -- AMP sub invoices. This table can be used to create a historical view of what
 -- happened to the node's invoices.

sqldb/sqlite.go

@@ -158,6 +158,10 @@ func (s *SqliteStore) ApplyAllMigrations(ctx context.Context,
 	return ApplyMigrations(ctx, s.BaseDB, s, migrations)
 }
 
+func errSqliteMigration(err error) error {
+	return fmt.Errorf("error creating sqlite migration: %w", err)
+}
+
 // ExecuteMigrations runs migrations for the sqlite database, depending on the
 // target given, either all migrations or up to a given version.
 func (s *SqliteStore) ExecuteMigrations(target MigrationTarget) error {
@@ -165,7 +169,7 @@ func (s *SqliteStore) ExecuteMigrations(target MigrationTarget) error {
 		s.DB, &sqlite_migrate.Config{},
 	)
 	if err != nil {
-		return fmt.Errorf("error creating sqlite migration: %w", err)
+		return errSqliteMigration(err)
 	}
 
 	// Populate the database with our set of schemas based on our embedded
@@ -176,6 +180,37 @@ func (s *SqliteStore) ExecuteMigrations(target MigrationTarget) error {
 	)
 }
 
+// GetSchemaVersion returns the current schema version of the SQLite database.
+func (s *SqliteStore) GetSchemaVersion() (int, bool, error) {
+	driver, err := sqlite_migrate.WithInstance(
+		s.DB, &sqlite_migrate.Config{},
+	)
+	if err != nil {
+		return 0, false, errSqliteMigration(err)
+	}
+
+	version, dirty, err := driver.Version()
+	if err != nil {
+		return 0, dirty, err
+	}
+
+	return version, dirty, nil
+}
+
+// SetSchemaVersion sets the schema version of the SQLite database.
+//
+// NOTE: This alters the internal database schema tracker. USE WITH CAUTION!!!
+func (s *SqliteStore) SetSchemaVersion(version int, dirty bool) error {
+	driver, err := sqlite_migrate.WithInstance(
+		s.DB, &sqlite_migrate.Config{},
+	)
+	if err != nil {
+		return errSqliteMigration(err)
+	}
+
+	return driver.SetVersion(version, dirty)
+}
+
 // NewTestSqliteDB is a helper function that creates an SQLite database for
 // testing.
 func NewTestSqliteDB(t *testing.T) *SqliteStore {