From a4c5ce682ba3c70e52ff4607371a3463ac7907c6 Mon Sep 17 00:00:00 2001
From: Garry Hill <garry@electric-sql.com>
Date: Mon, 18 Sep 2023 13:14:39 +0100
Subject: [PATCH] fix(electric): Prevent creation of duplicate DDL commands in
 migration history (#439)

---
 .changeset/rich-islands-hunt.md               |  5 ++
 .../lib/electric/postgres/extension.ex        |  3 +-
 ...5714_add_unique_constraint_ddl_commands.ex | 50 +++++++++++++++++++
 .../postgres/extension/ddl_capture_test.exs   | 20 ++++++++
 4 files changed, 77 insertions(+), 1 deletion(-)
 create mode 100644 .changeset/rich-islands-hunt.md
 create mode 100644 components/electric/lib/electric/postgres/extension/migrations/20230918115714_add_unique_constraint_ddl_commands.ex

diff --git a/.changeset/rich-islands-hunt.md b/.changeset/rich-islands-hunt.md
new file mode 100644
index 0000000000..a1464b8e26
--- /dev/null
+++ b/.changeset/rich-islands-hunt.md
@@ -0,0 +1,5 @@
+---
+"@core/electric": patch
+---
+
+VAX-1036 - fixes bugs reported by @hugodutka by preventing insertion of duplicate ddl commands in migration history
diff --git a/components/electric/lib/electric/postgres/extension.ex b/components/electric/lib/electric/postgres/extension.ex
index a474d6d26b..d4dfff7042 100644
--- a/components/electric/lib/electric/postgres/extension.ex
+++ b/components/electric/lib/electric/postgres/extension.ex
@@ -247,7 +247,8 @@ defmodule Electric.Postgres.Extension do
       Migrations.Migration_20230424154425_DDLX,
       Migrations.Migration_20230512000000_conflict_resolution_triggers,
       Migrations.Migration_20230605141256_ElectrifyFunction,
-      Migrations.Migration_20230715000000_UtilitiesTable
+      Migrations.Migration_20230715000000_UtilitiesTable,
+      Migrations.Migration_20230918115714_DDLCommandUniqueConstraint
     ]
   end
 
diff --git a/components/electric/lib/electric/postgres/extension/migrations/20230918115714_add_unique_constraint_ddl_commands.ex b/components/electric/lib/electric/postgres/extension/migrations/20230918115714_add_unique_constraint_ddl_commands.ex
new file mode 100644
index 0000000000..07de1da91e
--- /dev/null
+++ b/components/electric/lib/electric/postgres/extension/migrations/20230918115714_add_unique_constraint_ddl_commands.ex
@@ -0,0 +1,50 @@
+defmodule Electric.Postgres.Extension.Migrations.Migration_20230918115714_DDLCommandUniqueConstraint do
+  alias Electric.Postgres.Extension
+
+  @behaviour Extension.Migration
+
+  @txid_type "xid8"
+
+  @impl true
+  def version, do: 2023_09_18_11_57_14
+
+  @impl true
+  def up(schema) do
+    ddl_table = Extension.ddl_table()
+
+    [
+      """
+      ALTER TABLE #{ddl_table}
+        ADD CONSTRAINT ddl_table_unique_migrations
+        UNIQUE (txid, txts ,version, query); 
+      """,
+      """
+      CREATE OR REPLACE FUNCTION #{schema}.create_active_migration(
+          _txid #{@txid_type},
+          _txts timestamptz,
+          _version text,
+          _query text DEFAULT NULL
+      ) RETURNS int8 AS
+      $function$
+      DECLARE
+          trid int8;
+      BEGIN
+          IF _query IS NULL THEN
+              _query := current_query();
+          END IF;
+          RAISE NOTICE 'capture migration: % => %', _version, _query;
+          INSERT INTO #{ddl_table} (txid, txts, version, query) VALUES
+                (_txid, _txts, _version, _query)
+              ON CONFLICT ON CONSTRAINT ddl_table_unique_migrations DO NOTHING
+              RETURNING id INTO trid;
+          RETURN trid;
+      END;
+      $function$
+      LANGUAGE PLPGSQL;
+      """
+    ]
+  end
+
+  @impl true
+  def down(_), do: []
+end
diff --git a/components/electric/test/electric/postgres/extension/ddl_capture_test.exs b/components/electric/test/electric/postgres/extension/ddl_capture_test.exs
index abbc7cc5d5..c1465fb100 100644
--- a/components/electric/test/electric/postgres/extension/ddl_capture_test.exs
+++ b/components/electric/test/electric/postgres/extension/ddl_capture_test.exs
@@ -116,4 +116,24 @@ defmodule Electric.Postgres.Extension.DDLCaptureTest do
 
     assert {:error, _error} = :epgsql.squery(conn, sql4)
   end
+
+  test_tx "ADD column; CREATE INDEX only adds a single migration", fn conn ->
+    sql1 =
+      "CREATE TABLE public.buttercup (id text PRIMARY KEY, value text);"
+
+    sql2 = "CALL electric.electrify('public.buttercup')"
+
+    for sql <- [sql1, sql2] do
+      {:ok, _cols, _rows} = :epgsql.squery(conn, sql)
+    end
+
+    assert {:ok, [_]} = Extension.ddl_history(conn)
+
+    sql3 =
+      "ALTER TABLE public.buttercup ADD COLUMN amount int4; CREATE INDEX buttercup_amount_idx ON public.buttercup (amount); "
+
+    [{:ok, _, _}, {:ok, _, _}] = :epgsql.squery(conn, sql3)
+
+    assert {:ok, [_, {_, _, _, ^sql3}]} = Extension.ddl_history(conn)
+  end
 end