Fix previous version detection in the presence of inferred DDL migrat…

…ions (#197) Update the definition of the `previous_version` function to use a recursive CTE to find the first non-'inferred' parent of the current version. This requires adding a new column to the `pgroll.migrations` table to record whether a migration is a pgroll migration or an inferred DDL change made outside of pgroll. Together, this means that the previous version schema is removed correctly when there have been inferred DDL changes between the current and previous pgroll migration. Fixes #196
xataio · Nov 3, 2023 · c029d5e · c029d5e
1 parent 2429a6b
commit c029d5e
Show file tree

Hide file tree

Showing 2 changed files with 105 additions and 32 deletions.
diff --git a/pkg/roll/execute_test.go b/pkg/roll/execute_test.go
@@ -62,43 +62,95 @@ func TestSchemaIsCreatedfterMigrationStart(t *testing.T) {
 func TestPreviousVersionIsDroppedAfterMigrationCompletion(t *testing.T) {
 	t.Parallel()
 
-	withMigratorAndConnectionToContainer(t, func(mig *roll.Roll, db *sql.DB) {
-		ctx := context.Background()
-		const (
-			firstVersion  = "1_create_table"
-			secondVersion = "2_create_table"
-		)
+	t.Run("when the previous version is a pgroll migration", func(t *testing.T) {
+		withMigratorAndConnectionToContainer(t, func(mig *roll.Roll, db *sql.DB) {
+			ctx := context.Background()
+			const (
+				firstVersion  = "1_create_table"
+				secondVersion = "2_create_table"
+			)
+
+			if err := mig.Start(ctx, &migrations.Migration{Name: firstVersion, Operations: migrations.Operations{createTableOp("table1")}}); err != nil {
+				t.Fatalf("Failed to start first migration: %v", err)
+			}
+			if err := mig.Complete(ctx); err != nil {
+				t.Fatalf("Failed to complete first migration: %v", err)
+			}
+			if err := mig.Start(ctx, &migrations.Migration{Name: secondVersion, Operations: migrations.Operations{createTableOp("table2")}}); err != nil {
+				t.Fatalf("Failed to start second migration: %v", err)
+			}
+			if err := mig.Complete(ctx); err != nil {
+				t.Fatalf("Failed to complete second migration: %v", err)
+			}
 
-		if err := mig.Start(ctx, &migrations.Migration{Name: firstVersion, Operations: migrations.Operations{createTableOp("table1")}}); err != nil {
-			t.Fatalf("Failed to start first migration: %v", err)
-		}
-		if err := mig.Complete(ctx); err != nil {
-			t.Fatalf("Failed to complete first migration: %v", err)
-		}
-		if err := mig.Start(ctx, &migrations.Migration{Name: secondVersion, Operations: migrations.Operations{createTableOp("table2")}}); err != nil {
-			t.Fatalf("Failed to start second migration: %v", err)
-		}
-		if err := mig.Complete(ctx); err != nil {
-			t.Fatalf("Failed to complete second migration: %v", err)
-		}
+			//
+			// Check that the schema for the first version has been dropped
+			//
+			var exists bool
+			err := db.QueryRow(`
+    SELECT EXISTS(
+      SELECT 1
+      FROM pg_catalog.pg_namespace
+      WHERE nspname = $1
+    )`, roll.VersionedSchemaName(schema, firstVersion)).Scan(&exists)
+			if err != nil {
+				t.Fatal(err)
+			}
 
-		//
-		// Check that the schema for the first version has been dropped
-		//
-		var exists bool
-		err := db.QueryRow(`
+			if exists {
+				t.Errorf("Expected schema %q to not exist", firstVersion)
+			}
+		})
+	})
+
+	t.Run("when the previous version is an inferred DDL migration", func(t *testing.T) {
+		withMigratorAndConnectionToContainer(t, func(mig *roll.Roll, db *sql.DB) {
+			ctx := context.Background()
+			const (
+				firstVersion  = "1_create_table"
+				secondVersion = "2_create_table"
+			)
+
+			// Run the first pgroll migration
+			if err := mig.Start(ctx, &migrations.Migration{Name: firstVersion, Operations: migrations.Operations{createTableOp("table1")}}); err != nil {
+				t.Fatalf("Failed to start first migration: %v", err)
+			}
+			if err := mig.Complete(ctx); err != nil {
+				t.Fatalf("Failed to complete first migration: %v", err)
+			}
+
+			// Run a manual DDL migration
+			_, err := db.ExecContext(ctx, "CREATE TABLE foo (id integer)")
+			if err != nil {
+				t.Fatalf("Failed to create table: %v", err)
+			}
+
+			// Run the second pgroll migration
+			if err := mig.Start(ctx, &migrations.Migration{Name: secondVersion, Operations: migrations.Operations{createTableOp("table2")}}); err != nil {
+				t.Fatalf("Failed to start second migration: %v", err)
+			}
+			if err := mig.Complete(ctx); err != nil {
+				t.Fatalf("Failed to complete second migration: %v", err)
+			}
+
+			//
+			// Check that the schema for the first version has been dropped
+			//
+			var exists bool
+			err = db.QueryRow(`
     SELECT EXISTS(
       SELECT 1
       FROM pg_catalog.pg_namespace
       WHERE nspname = $1
     )`, roll.VersionedSchemaName(schema, firstVersion)).Scan(&exists)
-		if err != nil {
-			t.Fatal(err)
-		}
+			if err != nil {
+				t.Fatal(err)
+			}
 
-		if exists {
-			t.Errorf("Expected schema %q to not exist", firstVersion)
-		}
+			if exists {
+				t.Errorf("Expected schema %q to not exist", firstVersion)
+			}
+		})
 	})
 }
 

diff --git a/pkg/state/state.go b/pkg/state/state.go
@@ -41,6 +41,13 @@ CREATE UNIQUE INDEX IF NOT EXISTS only_first_migration_without_parent ON %[1]s.m
 -- History is linear
 CREATE UNIQUE INDEX IF NOT EXISTS history_is_linear ON %[1]s.migrations (schema, parent);
 
+-- Add a column to tell whether the row represents an auto-detected DDL capture or a pgroll migration
+ALTER TABLE %[1]s.migrations ADD COLUMN IF NOT EXISTS migration_type
+  VARCHAR(32)
+  DEFAULT 'pgroll'
+  CONSTRAINT migration_type_check CHECK (migration_type IN ('pgroll', 'inferred')
+);
+
 -- Helper functions
 
 -- Are we in the middle of a migration?
@@ -65,7 +72,20 @@ STABLE;
 -- Get the name of the previous version of the schema, or NULL if there is none.
 CREATE OR REPLACE FUNCTION %[1]s.previous_version(schemaname NAME) RETURNS text
 AS $$
-  SELECT parent FROM %[1]s.migrations WHERE name = (SELECT %[1]s.latest_version(schemaname)) AND schema=schemaname;
+  WITH RECURSIVE find_ancestor AS (
+    SELECT schema, name, parent, migration_type FROM pgroll.migrations 
+      WHERE name = (SELECT %[1]s.latest_version(schemaname)) AND schema = schemaname
+
+    UNION ALL
+
+    SELECT m.schema, m.name, m.parent, m.migration_type FROM pgroll.migrations m
+      INNER JOIN find_ancestor fa ON fa.parent = m.name AND fa.schema = m.schema
+      WHERE m.migration_type = 'inferred'
+  )
+  SELECT a.parent
+  FROM find_ancestor AS a
+  JOIN pgroll.migrations AS b ON a.parent = b.name AND a.schema = b.schema
+  WHERE b.migration_type = 'pgroll';
 $$
 LANGUAGE SQL
 STABLE;
@@ -192,14 +212,15 @@ BEGIN
 	END IF;
 
 	-- Someone did a schema change without pgroll, include it in the history
-	INSERT INTO %[1]s.migrations (schema, name, migration, resulting_schema, done, parent)
+	INSERT INTO %[1]s.migrations (schema, name, migration, resulting_schema, done, parent, migration_type)
 	VALUES (
 		schemaname,
 		pg_catalog.format('sql_%%s',pg_catalog.substr(pg_catalog.md5(pg_catalog.random()::text), 0, 15)),
 		pg_catalog.json_build_object('sql', pg_catalog.json_build_object('up', pg_catalog.current_query())),
 		%[1]s.read_schema(schemaname),
 		true,
-		%[1]s.latest_version(schemaname)
+		%[1]s.latest_version(schemaname),
+		'inferred'
 	);
 END;
 $$;