fix(electric): Allow creation of triggers on electrified tables (#663)

and other sundry table maintainance actions that don't affect table
structure

fixes #652

Author: magnetised

components/electric/lib/electric/postgres/proxy/query_analyser.ex

@@ -125,6 +125,34 @@ end
 defimpl QueryAnalyser, for: PgQuery.AlterTableStmt do
   import QueryAnalyser.Impl
 
+  @allowed_subtypes [
+    :AT_EnableAlwaysTrig,
+    :AT_EnableTrig,
+    :AT_EnableReplicaTrig,
+    :AT_DisableTrig,
+    :AT_ChangeOwner,
+    :AT_SetStorage,
+    :AT_SetStatistics,
+    :AT_SetOptions,
+    :AT_ResetOptions,
+    :AT_SetCompression,
+    :AT_SetRelOptions,
+    :AT_ResetRelOptions,
+    :AT_SetAccessMethod
+  ]
+
+  # actions that we maybe could support but are currently disabled:
+  #
+  # - DROP CONSTRAINT
+  # - DISABLE/ENABLE ROW LEVEL SECURITY
+  # - NO FORCE/FORCE ROW LEVEL SECURITY
+  # - CLUSTER ON
+  # - SET WITHOUT CLUSTER
+  # - SET TABLESPACE
+  #
+  # there's a bunch of stuff to do with inheritance, but my instinct is that
+  # allowing that would cause a lot of problems
+
   def analyse(stmt, %QueryAnalysis{} = analysis, _state) do
     stmt.cmds
     |> Enum.map(&unwrap_node/1)
@@ -175,6 +203,11 @@ defimpl QueryAnalyser, for: PgQuery.AlterTableStmt do
      }}
   end
 
+  defp analyse_alter_table_cmd(%{subtype: subtype}, analysis)
+       when subtype in @allowed_subtypes do
+    {:cont, %{analysis | allowed?: analysis.allowed? && true}}
+  end
+
   defp analyse_alter_table_cmd(cmd, analysis) do
     {:halt, %{analysis | allowed?: false, error: error_for(cmd, analysis)}}
   end

components/electric/test/electric/postgres/proxy/injector_test.exs

@@ -575,6 +575,32 @@ defmodule Electric.Postgres.Proxy.InjectorTest do
       |> idle!()
     end
 
+    test "enable trigger", cxt do
+      query1 =
+        "create or replace function function1() returns trigger as $$ return true; $$ language pgpsql"
+
+      query2 =
+        "alter table public.truths enable always trigger function1"
+
+      query = Enum.join([query1 <> ";", query2 <> ";"], "\n")
+
+      cxt.injector
+      |> client(begin())
+      |> server(complete_ready("BEGIN"))
+      |> client(query(query), server: query(query1))
+      |> server(complete_ready("CREATE FUNCTION1"), server: query(query2))
+      |> server(complete_ready("ALTER TABLE"),
+        client: [
+          complete("CREATE FUNCTION1"),
+          complete("ALTER TABLE"),
+          ready(:tx)
+        ]
+      )
+      |> client(commit())
+      |> server(complete_ready("COMMIT", :idle))
+      |> idle!()
+    end
+
     test "drop random things", cxt do
       cxt.injector
       |> client(begin())

components/electric/test/electric/postgres/proxy/query_analyser_test.exs

@@ -820,6 +820,85 @@ defmodule Electric.Postgres.Proxy.QueryAnalyserTest do
                )
     end
 
+    test "ALTER TABLE ... {ENABLE | DISABLE} TRIGGER", cxt do
+      stmts = [
+        "ALTER TABLE public.truths ENABLE ALWAYS TRIGGER my_totally_valid_trigger",
+        "ALTER TABLE public.truths ENABLE TRIGGER my_totally_valid_trigger",
+        "ALTER TABLE public.truths ENABLE REPLICA TRIGGER my_totally_valid_trigger",
+        "ALTER TABLE public.truths DISABLE TRIGGER my_totally_valid_trigger"
+      ]
+
+      for stmt <- stmts do
+        assert [
+                 %QueryAnalysis{
+                   action: {:alter, "table"},
+                   allowed?: true,
+                   capture?: false
+                 }
+               ] = analyse(simple(stmt), cxt)
+      end
+    end
+
+    test "ALTER TABLE ... allowed variants", cxt do
+      stmts = [
+        "ALTER TABLE public.truths OWNER TO someone",
+        "ALTER TABLE public.truths OWNER TO CURRENT_ROLE",
+        "ALTER TABLE public.truths ALTER COLUMN something SET STORAGE PLAIN",
+        "ALTER TABLE public.truths ALTER COLUMN something SET STATISTICS 3",
+        "ALTER TABLE public.truths ALTER COLUMN something SET ( n_distinct = -1 )",
+        "ALTER TABLE public.truths ALTER COLUMN something RESET ( n_distinct )",
+        "ALTER TABLE public.truths ALTER COLUMN something SET STORAGE MAIN",
+        "ALTER TABLE public.truths ALTER COLUMN something SET COMPRESSION lz4",
+        "ALTER TABLE public.truths SET ( fillfactor = 10 )",
+        "ALTER TABLE public.truths RESET ( fillfactor )",
+        "ALTER TABLE public.truths SET ACCESS METHOD something"
+      ]
+
+      for stmt <- stmts do
+        assert [
+                 %QueryAnalysis{
+                   action: {:alter, "table"},
+                   allowed?: true,
+                   capture?: false
+                 }
+               ] = analyse(simple(stmt), cxt)
+      end
+    end
+
+    test "ALTER TABLE ... mix of allowed and disallowed", cxt do
+      assert [
+               %QueryAnalysis{
+                 action: {:alter, "table"},
+                 allowed?: false,
+                 capture?: false
+               }
+             ] =
+               analyse(
+                 simple(
+                   "alter table public.truths " <>
+                     "enable always trigger my_totally_valid_trigger, " <>
+                     "drop column hat"
+                 ),
+                 cxt
+               )
+
+      assert [
+               %QueryAnalysis{
+                 action: {:alter, "table"},
+                 allowed?: false,
+                 capture?: false
+               }
+             ] =
+               analyse(
+                 simple(
+                   "alter table public.truths " <>
+                     "drop column hat, " <>
+                     "enable always trigger my_totally_valid_trigger"
+                 ),
+                 cxt
+               )
+    end
+
     # TODO: how to handle invalid sql (that doesn't include electric syntax)?
     #       ideally we'd want to forward to pg so it can give proper
     #       error messages