chore(electric): Validate column types when electrifying a table (#331)

Closes VAX-616.

.changeset/tasty-chairs-beg.md

@@ -0,0 +1,5 @@
+---
+"@core/electric": patch
+---
+
+Add a validation step in the electrify() function that only lets tables with supported column types to be electrified

components/electric/lib/electric/postgres/extension/migrations/20230605141256_electrify_function.ex

@@ -5,9 +5,9 @@ defmodule Electric.Postgres.Extension.Migrations.Migration_20230605141256_Electr
 
   @behaviour Extension.Migration
 
-  sql_file = Path.expand("20230605141256_electrify_function/electrify.sql", __DIR__)
+  sql_template = Path.expand("20230605141256_electrify_function/electrify.sql.eex", __DIR__)
 
-  @external_resource sql_file
+  @external_resource sql_template
 
   @impl true
   def version, do: 2023_06_05_14_12_56
@@ -18,17 +18,21 @@ defmodule Electric.Postgres.Extension.Migrations.Migration_20230605141256_Electr
     electrified_index_table = Extension.electrified_index_table()
     publication = Extension.publication_name()
     event_triggers = Extension.event_triggers()
+    event_trigger_tags = ["'ALTER TABLE'", "'DROP TABLE'", "'DROP INDEX'", "'DROP VIEW'"]
 
-    event_trigger_tags =
-      ["'ALTER TABLE'"] ++ for obj <- ["TABLE", "INDEX", "VIEW"], do: "'DROP #{obj}'"
+    supported_types_sql =
+      Electric.Satellite.Serialization.supported_pg_types()
+      |> Enum.map(&"'#{&1}'")
+      |> Enum.join(",")
 
     electrify_function =
       electrify_function_sql(
         schema,
         electrified_tracking_table,
         Extension.electrified_index_table(),
         publication,
-        Extension.add_table_to_publication_sql("%I.%I")
+        Extension.add_table_to_publication_sql("%I.%I"),
+        supported_types_sql
       )
 
     [
@@ -57,7 +61,7 @@ defmodule Electric.Postgres.Extension.Migrations.Migration_20230605141256_Electr
       electrify_function,
       """
       CREATE EVENT TRIGGER #{event_triggers[:sql_drop]} ON sql_drop
-          WHEN TAG IN (#{Enum.join(event_trigger_tags, ", ")}) 
+          WHEN TAG IN (#{Enum.join(event_trigger_tags, ", ")})
           EXECUTE FUNCTION #{schema}.ddlx_sql_drop_handler();
       """
     ]
@@ -68,11 +72,12 @@ defmodule Electric.Postgres.Extension.Migrations.Migration_20230605141256_Electr
     []
   end
 
-  EEx.function_from_file(:defp, :electrify_function_sql, sql_file, [
+  EEx.function_from_file(:defp, :electrify_function_sql, sql_template, [
     :schema,
     :electrified_tracking_table,
     :electrified_index_table,
     :publication_name,
-    :publication_sql
+    :publication_sql,
+    :valid_column_types
   ])
 end

components/electric/lib/electric/postgres/extension/migrations/20230605141256_electrify_function/electrify.sql → components/electric/lib/electric/postgres/extension/migrations/20230605141256_electrify_function/electrify.sql.eex

@@ -91,6 +91,39 @@ $function$ LANGUAGE PLPGSQL STABLE;
 
 -------------------------------------------------
 
+CREATE OR REPLACE FUNCTION <%= schema %>.__validate_table_column_types(table_name text)
+RETURNS VOID AS $function$
+DECLARE
+    _col_name text;
+    _col_type text;
+    _col_typmod int;
+    _col_type_pretty text;
+    _invalid_cols text[];
+BEGIN
+    FOR _col_name, _col_type, _col_typmod, _col_type_pretty IN
+        SELECT attname, typname, atttypmod, format_type(atttypid, atttypmod)
+            FROM pg_attribute
+            JOIN pg_type on atttypid = pg_type.oid
+            WHERE attrelid = table_name::regclass AND attnum > 0 AND NOT attisdropped
+            ORDER BY attnum
+    LOOP
+        IF _col_type NOT IN (<%= valid_column_types %>)
+           -- We only support unsized varchar type
+           OR ('varchar' IN (<%= valid_column_types %>) AND _col_type = 'varchar' AND _col_typmod <> -1)
+        THEN
+            _invalid_cols = array_append(_invalid_cols, format('"%s" %s', _col_name, _col_type_pretty));
+        END IF;
+    END LOOP;
+
+    IF _invalid_cols IS NOT NULL THEN
+        RAISE EXCEPTION E'Cannot electrify "%" because some of its columns have types not supported by Electric:\n  %',
+                        table_name, array_to_string(_invalid_cols, E'\n  ');
+    END IF;
+END;
+$function$ LANGUAGE PLPGSQL;
+
+-------------------------------------------------
+
 CREATE OR REPLACE PROCEDURE <%= schema %>.electrify(
     name1 text,
     name2 text DEFAULT NULL
@@ -117,6 +150,8 @@ BEGIN
         RAISE EXCEPTION '% is not an ordinary table', _quoted_name;
     END IF;
 
+    PERFORM <%= schema %>.__validate_table_column_types(_quoted_name);
+
     EXECUTE format('ALTER TABLE %I.%I REPLICA IDENTITY FULL;', _schema, _table);
 
     INSERT INTO <%= electrified_tracking_table %> (schema_name, table_name, oid)
@@ -133,9 +168,8 @@ BEGIN
 
     IF NOT EXISTS (
         SELECT pr.oid FROM pg_publication_rel pr
-        INNER JOIN pg_publication pp ON pr.prpubid = pp.oid
-        WHERE pp.pubname = '<%= publication_name %>'
-        AND pr.prrelid = _oid
+            INNER JOIN pg_publication pp ON pr.prpubid = pp.oid
+            WHERE pp.pubname = '<%= publication_name %>' AND pr.prrelid = _oid
         ) THEN
         EXECUTE format('<%= publication_sql %>;', _schema, _table);
 

components/electric/lib/electric/postgres/extension/migrations/20230715000000_utilities_table.ex

@@ -8,17 +8,19 @@ defmodule Electric.Postgres.Extension.Migrations.Migration_20230715000000_Utilit
 
   @impl true
   def up(_) do
+    table = Extension.transaction_marker_table()
+
     [
       """
-      CREATE TABLE #{Extension.transaction_marker_table()} (
+      CREATE TABLE #{table} (
         id VARCHAR(64) PRIMARY KEY,
         content jsonb NULL
       )
       """,
       """
-      INSERT INTO #{Extension.transaction_marker_table()} (id, content) VALUES ('magic write', '{}')
+      INSERT INTO #{table} (id, content) VALUES ('magic write', '{}')
       """,
-      Extension.add_table_to_publication_sql(Extension.transaction_marker_table())
+      Extension.add_table_to_publication_sql(table)
     ]
   end
 

components/electric/lib/electric/satellite/serialization.ex

@@ -19,6 +19,18 @@ defmodule Electric.Satellite.Serialization do
   @type relation_mapping() ::
           %{Changes.relation() => {PB.relation_id(), [Replication.Column.name()]}}
 
+  @spec supported_pg_types :: [atom]
+  def supported_pg_types do
+    ~w[
+      bytea
+      int2 int4 int8
+      float8
+      text
+      uuid
+      varchar
+    ]a
+  end
+
   @doc """
   Serialize from internal format to Satellite PB format
   """
@@ -417,8 +429,7 @@ defmodule Electric.Satellite.Serialization do
   """
   @spec decode_column_value(binary, atom) :: binary
 
-  def decode_column_value(val, type) when type in [:text, :bpchar, :varchar, :bytea] do
-    # TODO: validate value length for sized bpchar and varchar types.
+  def decode_column_value(val, type) when type in [:bytea, :text, :varchar] do
     val
   end
 

components/electric/test/electric/postgres/extension/electrify_test.exs

@@ -34,7 +34,7 @@ defmodule Electric.Postgres.Extension.ElectrifyTest do
     CREATE TABLE buttercup (
       id uuid PRIMARY KEY DEFAULT uuid_generate_v4(),
       value text,
-      secret bool
+      secret integer
     );
     """
 
@@ -58,7 +58,7 @@ defmodule Electric.Postgres.Extension.ElectrifyTest do
     CREATE TABLE buttercup (
       id uuid PRIMARY KEY DEFAULT uuid_generate_v4(),
       value text,
-      secret bool
+      secret integer
     );
     """
 
@@ -125,7 +125,7 @@ defmodule Electric.Postgres.Extension.ElectrifyTest do
     CREATE TABLE balloons.buttercup (
       id uuid PRIMARY KEY DEFAULT uuid_generate_v4(),
       value text,
-      secret bool
+      secret integer
     );
     """
 
@@ -187,7 +187,7 @@ defmodule Electric.Postgres.Extension.ElectrifyTest do
     CREATE TABLE buttercup (
       id uuid PRIMARY KEY DEFAULT uuid_generate_v4(),
       value text,
-      secret bool
+      secret integer
     );
     """
 

components/electric/test/electric/postgres/extension_test.exs

@@ -422,6 +422,54 @@ defmodule Electric.Postgres.ExtensionTest do
                )
     end
 
+    test_tx "table electrification successfully validates column types", fn conn ->
+      assert [{:ok, [], []}, {:ok, [], []}] ==
+               :epgsql.squery(conn, """
+               CREATE TABLE public.t1 (
+                 id UUID PRIMARY KEY,
+                 content TEXT NOT NULL,
+                 words VARCHAR,
+                 num2a INT2,
+                 num2b SMALLINT,
+                 num4a INT4,
+                 num4b INT,
+                 num4c INTEGER,
+                 num8a INT8,
+                 num8b BIGINT,
+                 real8a FLOAT8,
+                 real8b DOUBLE PRECISION
+               );
+               CALL electric.electrify('public.t1');
+               """)
+    end
+
+    test_tx "table electrification rejects invalid column types", fn conn ->
+      assert [
+               {:ok, [], []},
+               {:error, {:error, :error, _, :raise_exception, error_msg, _}}
+             ] =
+               :epgsql.squery(conn, """
+               CREATE TABLE public.t1 (
+                 id UUID PRIMARY KEY,
+                 c1 CHARACTER,
+                 c2 CHARACTER(11),
+                 c3 VARCHAR(11),
+                 created_at TIMESTAMP
+               );
+               CALL electric.electrify('public.t1');
+               """)
+
+      assert error_msg ==
+               """
+               Cannot electrify "public.t1" because some of its columns have types not supported by Electric:
+                 "c1" character(1)
+                 "c2" character(11)
+                 "c3" character varying(11)
+                 "created_at" timestamp without time zone
+               """
+               |> String.trim()
+    end
+
     test_tx "electrified?/2", fn conn ->
       sql1 = "CREATE TABLE public.buttercup (id int8 GENERATED ALWAYS AS IDENTITY PRIMARY KEY);"
       sql2 = "CREATE TABLE public.daisy (id int8 GENERATED ALWAYS AS IDENTITY PRIMARY KEY);"

components/electric/test/electric/postgres_test.exs

@@ -5,7 +5,7 @@ defmodule Electric.PostgresTest do
   import Electric.Postgres.TestConnection
 
   setup do
-    context = setup_test_db()
+    context = create_test_db()
 
     # start the replication process here so that it will be stopped before
     # we get to the on_exit handler defined in the setup_all

components/electric/test/electric/satellite/ws_validations_test.exs

@@ -14,6 +14,7 @@ defmodule Electric.Satellite.WsValidationsTest do
 
   @ws_listener_name :ws_validations_test
   @table_name "foo"
+  @receive_timeout 500
 
   setup :setup_replicated_db
 
@@ -60,7 +61,7 @@ defmodule Electric.Satellite.WsValidationsTest do
       # Wait long enough for the server to process our messages, thus confirming it has been accepted
       ping_server(conn)
 
-      refute_receive {^conn, %SatErrorResp{error_type: :INVALID_REQUEST}}
+      refute_receive {^conn, %SatErrorResp{error_type: :INVALID_REQUEST}}, @receive_timeout
     end)
   end
 
@@ -85,7 +86,7 @@ defmodule Electric.Satellite.WsValidationsTest do
       within_replication_context(ctx, vsn, fn conn ->
         tx_op_log = serialize_trans(record)
         MockClient.send_data(conn, tx_op_log)
-        assert_receive {^conn, %SatErrorResp{error_type: :INVALID_REQUEST}}
+        assert_receive {^conn, %SatErrorResp{error_type: :INVALID_REQUEST}}, @receive_timeout
       end)
     end)
   end
@@ -139,7 +140,7 @@ defmodule Electric.Satellite.WsValidationsTest do
       within_replication_context(ctx, vsn, fn conn ->
         tx_op_log = serialize_trans(record)
         MockClient.send_data(conn, tx_op_log)
-        assert_receive {^conn, %SatErrorResp{error_type: :INVALID_REQUEST}}
+        assert_receive {^conn, %SatErrorResp{error_type: :INVALID_REQUEST}}, @receive_timeout
       end)
     end)
   end
@@ -194,7 +195,7 @@ defmodule Electric.Satellite.WsValidationsTest do
       within_replication_context(ctx, vsn, fn conn ->
         tx_op_log = serialize_trans(record)
         MockClient.send_data(conn, tx_op_log)
-        assert_receive {^conn, %SatErrorResp{error_type: :INVALID_REQUEST}}
+        assert_receive {^conn, %SatErrorResp{error_type: :INVALID_REQUEST}}, @receive_timeout
       end)
     end)
   end
@@ -233,7 +234,7 @@ defmodule Electric.Satellite.WsValidationsTest do
       within_replication_context(ctx, vsn, fn conn ->
         tx_op_log = serialize_trans(record)
         MockClient.send_data(conn, tx_op_log)
-        assert_receive {^conn, %SatErrorResp{error_type: :INVALID_REQUEST}}
+        assert_receive {^conn, %SatErrorResp{error_type: :INVALID_REQUEST}}, @receive_timeout
       end)
     end)
   end

components/electric/test/support/postgres_test_connection.ex

@@ -46,7 +46,7 @@ defmodule Electric.Postgres.TestConnection do
     end)
   end
 
-  def setup_test_db(setup_fun \\ fn _ -> nil end, teardown_fun \\ fn _ -> nil end) do
+  def create_test_db(setup_fun \\ fn _ -> nil end, teardown_fun \\ fn _ -> nil end) do
     db_name = "electric_postgres_test_#{DateTime.utc_now() |> DateTime.to_unix()}"
     config = config() |> Keyword.delete(:database)
 
@@ -109,7 +109,7 @@ defmodule Electric.Postgres.TestConnection do
       )
     end
 
-    context = Map.merge(context, setup_test_db(setup_fun, teardown_fun))
+    context = Map.merge(context, create_test_db(setup_fun, teardown_fun))
 
     pg_connector_opts =
       context
@@ -160,8 +160,8 @@ defmodule Electric.Postgres.TestConnection do
       :epgsql.squery(conn, """
       CREATE TABLE public.my_entries (
         id UUID PRIMARY KEY DEFAULT uuid_generate_v4(),
-        content VARCHAR(64) NOT NULL,
-        content_b VARCHAR(64)
+        content VARCHAR NOT NULL,
+        content_b TEXT
       );
 
       """)

e2e/init.sql

@@ -1,11 +1,11 @@
 CREATE TABLE entries (
   id UUID PRIMARY KEY DEFAULT uuid_generate_v4(),
-  content VARCHAR(64) NOT NULL,
-  content_b VARCHAR(64)
+  content VARCHAR NOT NULL,
+  content_b TEXT
 );
 
 CREATE TABLE owned_entries (
   id UUID PRIMARY KEY DEFAULT uuid_generate_v4(),
-  electric_user_id VARCHAR(255) NOT NULL,
-  content VARCHAR(64) NOT NULL
+  electric_user_id TEXT NOT NULL,
+  content VARCHAR NOT NULL
 );

e2e/tests/01.04_postgres_alter_table_also_alters_shadows.lux

@@ -6,7 +6,7 @@
 
 [shell pg_1]
     !BEGIN;
-    !ALTER TABLE entries ADD COLUMN new_column VARCHAR(64);
+    !ALTER TABLE entries ADD COLUMN new_column VARCHAR;
     !COMMIT;
     ?$psql
     # Verify column structure of the shadow table:
@@ -16,4 +16,4 @@
     ?$psql
 
 [cleanup]
-    [invoke teardown]
+    [invoke teardown]