Merge branch 'smh-delete-orphaned-records' into 'master'

Make repository ID non nullable

See merge request gitlab-org/gitaly!4045

4 files changed, 118 insertions, 46 deletions

diff --git a/internal/praefect/datastore/listener_postgres_test.go b/internal/praefect/datastore/listener_postgres_test.go
index 53f1cc403..66378320f 100644
--- a/internal/praefect/datastore/listener_postgres_test.go
+++ b/internal/praefect/datastore/listener_postgres_test.go
@@ -378,10 +378,10 @@ func TestPostgresListener_Listen_repositories_delete(t *testing.T) {
 		func(t *testing.T) {
 			_, err := db.DB.Exec(`
 				INSERT INTO repositories
-				VALUES ('praefect-1', '/path/to/repo/1', 1),
-					('praefect-1', '/path/to/repo/2', 1),
-					('praefect-1', '/path/to/repo/3', 0),
-					('praefect-2', '/path/to/repo/1', 1)`)
+				VALUES ('praefect-1', '/path/to/repo/1', 1, 1),
+					('praefect-1', '/path/to/repo/2', 1, 2),
+					('praefect-1', '/path/to/repo/3', 0, 3),
+					('praefect-2', '/path/to/repo/1', 1, 4)`)
 			require.NoError(t, err)
 		},
 		func(t *testing.T) {
@@ -404,16 +404,22 @@ func TestPostgresListener_Listen_storage_repositories_insert(t *testing.T) {
 
 	const channel = "storage_repositories_updates"
 
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	testListener(
 		t,
 		db.Name,
 		channel,
-		func(t *testing.T) {},
+		func(t *testing.T) {
+			rs := NewPostgresRepositoryStore(db, nil)
+			require.NoError(t, rs.CreateRepository(ctx, 1, "praefect-1", "/path/to/repo", "replica-path", "primary", nil, nil, true, false))
+		},
 		func(t *testing.T) {
 			_, err := db.DB.Exec(`
 				INSERT INTO storage_repositories
-				VALUES ('praefect-1', '/path/to/repo', 'gitaly-1', 0),
-					('praefect-1', '/path/to/repo', 'gitaly-2', 0)`,
+				VALUES ('praefect-1', '/path/to/repo', 'gitaly-1', 0, 1),
+					('praefect-1', '/path/to/repo', 'gitaly-2', 0, 1)`,
 			)
 			require.NoError(t, err)
 		},
@@ -430,13 +436,16 @@ func TestPostgresListener_Listen_storage_repositories_update(t *testing.T) {
 
 	const channel = "storage_repositories_updates"
 
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	testListener(
 		t,
 		db.Name,
 		channel,
 		func(t *testing.T) {
-			_, err := db.DB.Exec(`INSERT INTO storage_repositories VALUES ('praefect-1', '/path/to/repo', 'gitaly-1', 0)`)
-			require.NoError(t, err)
+			rs := NewPostgresRepositoryStore(db, nil)
+			require.NoError(t, rs.CreateRepository(ctx, 1, "praefect-1", "/path/to/repo", "replica-path", "gitaly-1", nil, nil, true, false))
 		},
 		func(t *testing.T) {
 			_, err := db.DB.Exec(`UPDATE storage_repositories SET generation = generation + 1`)
@@ -474,16 +483,16 @@ func TestPostgresListener_Listen_storage_repositories_delete(t *testing.T) {
 
 	const channel = "storage_repositories_updates"
 
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	testListener(
 		t,
 		db.Name,
 		channel,
 		func(t *testing.T) {
-			_, err := db.DB.Exec(`
-				INSERT INTO storage_repositories (virtual_storage, relative_path, storage, generation)
-				VALUES ('praefect-1', '/path/to/repo', 'gitaly-1', 0)`,
-			)
-			require.NoError(t, err)
+			rs := NewPostgresRepositoryStore(db, nil)
+			require.NoError(t, rs.CreateRepository(ctx, 1, "praefect-1", "/path/to/repo", "replica-path", "gitaly-1", nil, nil, true, false))
 		},
 		func(t *testing.T) {
 			_, err := db.DB.Exec(`DELETE FROM storage_repositories`)
diff --git a/internal/praefect/datastore/migrations/20211105100456_check_non_null_repository_id.go b/internal/praefect/datastore/migrations/20211105100456_check_non_null_repository_id.go
new file mode 100644
index 000000000..6cc68810d
--- /dev/null
+++ b/internal/praefect/datastore/migrations/20211105100456_check_non_null_repository_id.go
@@ -0,0 +1,21 @@
+package migrations
+
+import migrate "github.com/rubenv/sql-migrate"
+
+func init() {
+	m := &migrate.Migration{
+		Id: "20211105100456_check_non_null_repository_id",
+		Up: []string{
+			// Set a non-validated NOT NULL constraint to ensure no new entries are inserted during the delete statement in the
+			// next migration that sets the columns as NOT NULL.
+			"ALTER TABLE storage_repositories ADD CONSTRAINT repository_id_not_null CHECK (repository_id IS NOT NULL) NOT VALID",
+			"ALTER TABLE repository_assignments ADD CONSTRAINT repository_id_not_null CHECK (repository_id IS NOT NULL) NOT VALID",
+		},
+		Down: []string{
+			"ALTER TABLE storage_repositories DROP CONSTRAINT repository_id_not_null",
+			"ALTER TABLE repository_assignments DROP CONSTRAINT repository_id_not_null",
+		},
+	}
+
+	allMigrations = append(allMigrations, m)
+}
diff --git a/internal/praefect/datastore/migrations/20211105100458_non_nullable_repository_id.go b/internal/praefect/datastore/migrations/20211105100458_non_nullable_repository_id.go
new file mode 100644
index 000000000..4b5e9478f
--- /dev/null
+++ b/internal/praefect/datastore/migrations/20211105100458_non_nullable_repository_id.go
@@ -0,0 +1,40 @@
+package migrations
+
+import migrate "github.com/rubenv/sql-migrate"
+
+func init() {
+	m := &migrate.Migration{
+		Id: "20211105100458_non_nullable_repository_id",
+		Up: []string{
+			// Disable the trigger to avoid notifications being sent for the delete below.
+			// No cache invalidation is necessary as the repository being deleted would have invalidated
+			// the cache and it wouldn't have been filled afterwards as the repository doesn't exist.
+			"ALTER TABLE storage_repositories DISABLE TRIGGER notify_on_delete",
+			// Remove orphaned replica records. 20210922143752_storage_repositories_cascade_delete introduced also
+			// in 14.5 changed the repository_id foreign key to cascade deletes from the `repositories` table.
+			// With the migration applied, the cascade in the schema guarantees that even older version of Praefect
+			// do not leave orphaned records around anymore when deleting repositories. This ensures concurrent deletions
+			// performed by older Praefects won't leave any orphaned records which would prevent the NOT NULL constraint
+			// from being set. This allows us to already clean up the old state in the same release the behavior change
+			// was introduced.
+			"DELETE FROM storage_repositories WHERE repository_id IS NULL",
+			// Enable the triggers again after the deletion is done.
+			"ALTER TABLE storage_repositories ENABLE TRIGGER notify_on_delete",
+			// With all the orphaned records gone, make the schema stricter and make the repository id
+			// non nullable.
+			"ALTER TABLE storage_repositories ALTER COLUMN repository_id SET NOT NULL",
+			"ALTER TABLE repository_assignments ALTER COLUMN repository_id SET NOT NULL",
+			// Drop the non validated constraint now that the column is set to NOT NULL.
+			"ALTER TABLE storage_repositories DROP CONSTRAINT repository_id_not_null",
+			"ALTER TABLE repository_assignments DROP CONSTRAINT repository_id_not_null",
+		},
+		Down: []string{
+			"ALTER TABLE repository_assignments ADD CONSTRAINT repository_id_not_null CHECK (repository_id IS NOT NULL) NOT VALID",
+			"ALTER TABLE storage_repositories ADD CONSTRAINT repository_id_not_null CHECK (repository_id IS NOT NULL) NOT VALID",
+			"ALTER TABLE storage_repositories ALTER COLUMN repository_id DROP NOT NULL",
+			"ALTER TABLE repository_assignments ALTER COLUMN repository_id DROP NOT NULL",
+		},
+	}
+
+	allMigrations = append(allMigrations, m)
+}
diff --git a/internal/praefect/nodes/sql_elector_test.go b/internal/praefect/nodes/sql_elector_test.go
index b86651e5b..65dcdf0d0 100644
--- a/internal/praefect/nodes/sql_elector_test.go
+++ b/internal/praefect/nodes/sql_elector_test.go
@@ -332,27 +332,27 @@ func TestElectNewPrimary(t *testing.T) {
 			desc: "gitaly-2 storage has more up to date repositories",
 			initialReplQueueInsert: `
 			INSERT INTO repositories
-				(virtual_storage, relative_path, generation)
+				(repository_id, virtual_storage, relative_path, generation)
 			VALUES
-				('test-shard-0', '/p/1', 5),
-				('test-shard-0', '/p/2', 5),
-				('test-shard-0', '/p/3', 5),
-				('test-shard-0', '/p/4', 5),
-				('test-shard-0', '/p/5', 5);
+				(1, 'test-shard-0', '/p/1', 5),
+				(2, 'test-shard-0', '/p/2', 5),
+				(3, 'test-shard-0', '/p/3', 5),
+				(4, 'test-shard-0', '/p/4', 5),
+				(5, 'test-shard-0', '/p/5', 5);
 
 			INSERT INTO storage_repositories
-				(virtual_storage, relative_path, storage, generation)
+				(repository_id, virtual_storage, relative_path, storage, generation)
 			VALUES
-				('test-shard-0', '/p/1', 'gitaly-1', 5),
-				('test-shard-0', '/p/2', 'gitaly-1', 5),
-				('test-shard-0', '/p/3', 'gitaly-1', 4),
-				('test-shard-0', '/p/4', 'gitaly-1', 3),
-
-				('test-shard-0', '/p/1', 'gitaly-2', 5),
-				('test-shard-0', '/p/2', 'gitaly-2', 5),
-				('test-shard-0', '/p/3', 'gitaly-2', 4),
-				('test-shard-0', '/p/4', 'gitaly-2', 4),
-				('test-shard-0', '/p/5', 'gitaly-2', 5)
+				(1, 'test-shard-0', '/p/1', 'gitaly-1', 5),
+				(2, 'test-shard-0', '/p/2', 'gitaly-1', 5),
+				(3, 'test-shard-0', '/p/3', 'gitaly-1', 4),
+				(4, 'test-shard-0', '/p/4', 'gitaly-1', 3),
+
+				(1, 'test-shard-0', '/p/1', 'gitaly-2', 5),
+				(2, 'test-shard-0', '/p/2', 'gitaly-2', 5),
+				(3, 'test-shard-0', '/p/3', 'gitaly-2', 4),
+				(4, 'test-shard-0', '/p/4', 'gitaly-2', 4),
+				(5, 'test-shard-0', '/p/5', 'gitaly-2', 5)
 			`,
 			expectedPrimary: "gitaly-2",
 			fallbackChoice:  false,
@@ -361,16 +361,17 @@ func TestElectNewPrimary(t *testing.T) {
 			desc: "gitaly-2 storage has less repositories as some may not been replicated yet",
 			initialReplQueueInsert: `
 			INSERT INTO REPOSITORIES
-				(virtual_storage, relative_path, generation)
+				(repository_id, virtual_storage, relative_path, generation)
 			VALUES
-				('test-shard-0', '/p/1', 5),
-				('test-shard-0', '/p/2', 5);
+				(1, 'test-shard-0', '/p/1', 5),
+				(2, 'test-shard-0', '/p/2', 5);
 
 			INSERT INTO STORAGE_REPOSITORIES
+				(repository_id, virtual_storage, relative_path, storage, generation)
 			VALUES
-				('test-shard-0', '/p/1', 'gitaly-1', 5),
-				('test-shard-0', '/p/2', 'gitaly-1', 4),
-				('test-shard-0', '/p/1', 'gitaly-2', 5)`,
+				(1, 'test-shard-0', '/p/1', 'gitaly-1', 5),
+				(2, 'test-shard-0', '/p/2', 'gitaly-1', 4),
+				(1, 'test-shard-0', '/p/1', 'gitaly-2', 5)`,
 			expectedPrimary: "gitaly-1",
 			fallbackChoice:  false,
 		},
@@ -378,19 +379,20 @@ func TestElectNewPrimary(t *testing.T) {
 			desc: "gitaly-1 is primary as it has less generations behind in total despite it has less repositories",
 			initialReplQueueInsert: `
 			INSERT INTO REPOSITORIES
-				(virtual_storage, relative_path, generation)
+				(repository_id, virtual_storage, relative_path, generation)
 			VALUES
-				('test-shard-0', '/p/1', 2),
-				('test-shard-0', '/p/2', 2),
-				('test-shard-0', '/p/3', 10);
+				(1, 'test-shard-0', '/p/1', 2),
+				(2, 'test-shard-0', '/p/2', 2),
+				(3, 'test-shard-0', '/p/3', 10);
 
 			INSERT INTO STORAGE_REPOSITORIES
+				(repository_id, virtual_storage, relative_path, storage, generation)
 			VALUES
-				('test-shard-0', '/p/2', 'gitaly-1', 1),
-				('test-shard-0', '/p/3', 'gitaly-1', 9),
-				('test-shard-0', '/p/1', 'gitaly-2', 1),
-				('test-shard-0', '/p/2', 'gitaly-2', 1),
-				('test-shard-0', '/p/3', 'gitaly-2', 1)`,
+				(2, 'test-shard-0', '/p/2', 'gitaly-1', 1),
+				(3, 'test-shard-0', '/p/3', 'gitaly-1', 9),
+				(1, 'test-shard-0', '/p/1', 'gitaly-2', 1),
+				(2, 'test-shard-0', '/p/2', 'gitaly-2', 1),
+				(3, 'test-shard-0', '/p/3', 'gitaly-2', 1)`,
 			expectedPrimary: "gitaly-1",
 			fallbackChoice:  false,
 		},