Skip to content

Commit d88225f

Browse files
RylandBangerter85cursoragentzachdaniel
authored
fix: show warning on concurrent index multitenancy #610 (#711)
* Fix concurrent index migrations with schema-driven multitenancy - Detect migrations with @disable_ddl_transaction attribute - Run concurrent index migrations outside transaction context - Use Ecto.Adapters.SQL.checkout to get fresh connection - Add comprehensive test suite for concurrent index scenarios - Maintain backward compatibility with regular migrations Fixes issue where CREATE INDEX CONCURRENTLY fails when creating tenants * Address PR feedback: use only module metadata for disable_ddl_transaction check Co-authored-by: Cursor <cursoragent@cursor.com> * Switch to warning approach: log when @disable_ddl_transaction runs in transaction Made-with: Cursor * Log single warning listing all affected modules instead of per-module Made-with: Cursor * Apply suggestion from @zachdaniel --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Zach Daniel <zachary.s.daniel@gmail.com>
1 parent e199457 commit d88225f

File tree

2 files changed

+252
-11
lines changed

2 files changed

+252
-11
lines changed

‎lib/multitenancy.ex‎

Lines changed: 49 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
defmodule AshPostgres.MultiTenancy do
66
@moduledoc false
77

8-
@dialyzer {:nowarn_function, load_migration!: 1}
98
require Logger
109

1110
# sobelow_skip ["SQL.Query"]
@@ -44,8 +43,36 @@ defmodule AshPostgres.MultiTenancy do
4443
end)
4544
|> Enum.map(&extract_migration_info/1)
4645
|> Enum.filter(& &1)
47-
|> Enum.map(&load_migration!/1)
48-
|> Enum.each(fn {version, mod} ->
46+
|> Enum.map(&load_migration_with_file!/1)
47+
|> then(fn migrations ->
48+
if repo.in_transaction?() do
49+
modules_requiring_no_transaction =
50+
migrations
51+
|> Enum.filter(fn {_version, mod, _file} -> migration_requires_no_transaction?(mod) end)
52+
|> Enum.map(fn {_version, mod, _file} -> mod end)
53+
54+
if Enum.any?(modules_requiring_no_transaction) do
55+
module_list =
56+
modules_requiring_no_transaction
57+
|> Enum.map(&inspect/1)
58+
|> Enum.join(", ")
59+
60+
Logger.warning("""
61+
Tenant migrations use @disable_ddl_transaction (e.g. CREATE INDEX CONCURRENTLY) but are \
62+
running inside a transaction. This will likely fail with "CREATE INDEX CONCURRENTLY cannot \
63+
run inside a transaction block".
64+
65+
Affected modules: #{module_list}
66+
67+
To fix this, ensure the action that creates/migrates tenants does not run inside a transaction. \
68+
For Ash resources with manage_tenant: true, set transaction?: false on the create/update action.
69+
""")
70+
end
71+
end
72+
73+
migrations
74+
end)
75+
|> Enum.each(fn {version, mod, _file} ->
4976
Ecto.Migration.Runner.run(
5077
repo,
5178
[],
@@ -74,19 +101,18 @@ defmodule AshPostgres.MultiTenancy do
74101
:ok
75102
end
76103

77-
defp load_migration!({version, _, file}) when is_binary(file) do
104+
defp load_migration_with_file!({version, _, file}) when is_binary(file) do
78105
loaded_modules = file |> compile_file() |> Enum.map(&elem(&1, 0))
79106

80-
case Enum.find(loaded_modules, &migration?/1) do
81-
nil ->
82-
raise Ecto.MigrationError,
83-
"file #{Path.relative_to_cwd(file)} does not define an Ecto.Migration"
84-
85-
mod ->
86-
{version, mod}
107+
if mod = Enum.find(loaded_modules, &migration?/1) do
108+
{version, mod, file}
109+
else
110+
raise Ecto.MigrationError,
111+
"file #{Path.relative_to_cwd(file)} does not define an Ecto.Migration"
87112
end
88113
end
89114

115+
90116
defp compile_file(file) do
91117
AshPostgres.MigrationCompileCache.start_link()
92118
AshPostgres.MigrationCompileCache.compile_file(file)
@@ -123,4 +149,16 @@ defmodule AshPostgres.MultiTenancy do
123149
defp tenant_name_regex do
124150
~r/^[a-zA-Z0-9_-]+$/
125151
end
152+
153+
# Check if a migration requires no transaction by examining the compiled module's
154+
# migration metadata (e.g. @disable_ddl_transaction for CREATE INDEX CONCURRENTLY).
155+
# Running such migrations inside a transaction will fail in PostgreSQL.
156+
defp migration_requires_no_transaction?(mod) do
157+
if function_exported?(mod, :__migration__, 0) do
158+
migration_info = mod.__migration__()
159+
Map.get(migration_info, :disable_ddl_transaction, false)
160+
else
161+
false
162+
end
163+
end
126164
end

‎test/concurrent_index_multitenancy_test.exs‎

Lines changed: 203 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,203 @@
1+
# SPDX-FileCopyrightText: 2019 ash_postgres contributors <https://github.com/ash-project/ash_postgres/graphs.contributors>
2+
#
3+
# SPDX-License-Identifier: MIT
4+
5+
defmodule AshPostgres.Test.ConcurrentIndexMultitenancyTest do
6+
use AshPostgres.RepoCase, async: false
7+
8+
import ExUnit.CaptureLog
9+
10+
alias AshPostgres.MultiTenancy
11+
12+
@temp_migrations_dir "priv/test_repo/temp_tenant_migrations"
13+
14+
setup do
15+
# Create temporary migrations directory
16+
File.mkdir_p!(@temp_migrations_dir)
17+
18+
# Ensure create_tenant!/2 and migrate_tenant/3 read tenant migrations from our temp directory.
19+
original_repo_env = Application.get_env(:ash_postgres, AshPostgres.TestRepo, [])
20+
21+
Application.put_env(
22+
:ash_postgres,
23+
AshPostgres.TestRepo,
24+
Keyword.put(original_repo_env, :tenant_migrations_path, @temp_migrations_dir)
25+
)
26+
27+
on_exit(fn ->
28+
File.rm_rf!(@temp_migrations_dir)
29+
Application.put_env(:ash_postgres, AshPostgres.TestRepo, original_repo_env)
30+
31+
test_tenants = [
32+
"test_tenant_regular",
33+
"test_tenant_warning"
34+
]
35+
36+
for tenant <- test_tenants do
37+
try do
38+
Ecto.Adapters.SQL.query!(
39+
AshPostgres.TestRepo,
40+
"DROP SCHEMA IF EXISTS \"#{tenant}\" CASCADE",
41+
[]
42+
)
43+
rescue
44+
_ -> :ok
45+
end
46+
end
47+
end)
48+
49+
:ok
50+
end
51+
52+
describe "tenant migrations with multitenancy" do
53+
test "migrate_tenant works with regular (non-concurrent) migration" do
54+
tenant_name = "test_tenant_regular"
55+
migration_file = Path.join(@temp_migrations_dir, "20250101000001_create_table_regular.exs")
56+
57+
migration_content = """
58+
defmodule AshPostgres.TestRepo.TempTenantMigrations.CreateTableRegular do
59+
use Ecto.Migration
60+
61+
def up do
62+
create table(:regular_table, primary_key: false, prefix: prefix()) do
63+
add :id, :uuid, null: false, default: fragment("uuid_generate_v4()"), primary_key: true
64+
add :name, :text
65+
add :status, :text
66+
end
67+
68+
create index(:regular_table, [:status], prefix: prefix())
69+
end
70+
71+
def down do
72+
drop index(:regular_table, [:status], prefix: prefix())
73+
drop table(:regular_table, prefix: prefix())
74+
end
75+
end
76+
"""
77+
78+
File.write!(migration_file, migration_content)
79+
80+
Ecto.Adapters.SQL.query!(
81+
AshPostgres.TestRepo,
82+
"CREATE SCHEMA IF NOT EXISTS \"#{tenant_name}\"",
83+
[]
84+
)
85+
86+
assert :ok = try_migrate_tenant(tenant_name, @temp_migrations_dir)
87+
88+
assert schema_exists?(tenant_name)
89+
assert table_exists?(tenant_name, "regular_table")
90+
assert index_exists?(tenant_name, "regular_table", "regular_table_status_index")
91+
end
92+
93+
test "logs warning when @disable_ddl_transaction migration runs inside a transaction" do
94+
tenant_name = "test_tenant_warning"
95+
migration_file = Path.join(@temp_migrations_dir, "20250101000001_create_table_with_attr.exs")
96+
97+
# Migration has @disable_ddl_transaction but only creates a table (no CONCURRENTLY).
98+
# This lets the migration succeed while still triggering the warning when run in a transaction.
99+
migration_content = """
100+
defmodule AshPostgres.TestRepo.TempTenantMigrations.CreateTableWithAttr do
101+
use Ecto.Migration
102+
103+
@disable_ddl_transaction true
104+
105+
def up do
106+
create table(:warning_test_table, primary_key: false, prefix: prefix()) do
107+
add :id, :uuid, null: false, default: fragment("uuid_generate_v4()"), primary_key: true
108+
add :name, :text
109+
end
110+
end
111+
112+
def down do
113+
drop table(:warning_test_table, prefix: prefix())
114+
end
115+
end
116+
"""
117+
118+
File.write!(migration_file, migration_content)
119+
120+
Ecto.Adapters.SQL.query!(
121+
AshPostgres.TestRepo,
122+
"CREATE SCHEMA IF NOT EXISTS \"#{tenant_name}\"",
123+
[]
124+
)
125+
126+
Ecto.Migration.SchemaMigration.ensure_schema_migrations_table!(
127+
AshPostgres.TestRepo,
128+
AshPostgres.TestRepo.config(),
129+
prefix: tenant_name
130+
)
131+
132+
log =
133+
capture_log(fn ->
134+
AshPostgres.TestRepo.transaction(fn ->
135+
MultiTenancy.migrate_tenant(tenant_name, AshPostgres.TestRepo, @temp_migrations_dir)
136+
end)
137+
end)
138+
139+
assert log =~ "@disable_ddl_transaction"
140+
assert log =~ "transaction"
141+
assert log =~ "transaction?: false"
142+
end
143+
end
144+
145+
defp try_migrate_tenant(tenant_name, migrations_path) do
146+
MultiTenancy.migrate_tenant(tenant_name, AshPostgres.TestRepo, migrations_path)
147+
:ok
148+
end
149+
150+
defp schema_exists?(schema_name) do
151+
result =
152+
Ecto.Adapters.SQL.query!(
153+
AshPostgres.TestRepo,
154+
"""
155+
SELECT schema_name
156+
FROM information_schema.schemata
157+
WHERE schema_name = $1
158+
""",
159+
[schema_name]
160+
)
161+
162+
case result.rows do
163+
[[^schema_name]] -> true
164+
_ -> false
165+
end
166+
end
167+
168+
defp table_exists?(schema_name, table_name) do
169+
result =
170+
Ecto.Adapters.SQL.query!(
171+
AshPostgres.TestRepo,
172+
"""
173+
SELECT table_name
174+
FROM information_schema.tables
175+
WHERE table_schema = $1 AND table_name = $2
176+
""",
177+
[schema_name, table_name]
178+
)
179+
180+
case result.rows do
181+
[[^table_name]] -> true
182+
_ -> false
183+
end
184+
end
185+
186+
defp index_exists?(schema_name, table_name, index_name) do
187+
result =
188+
Ecto.Adapters.SQL.query!(
189+
AshPostgres.TestRepo,
190+
"""
191+
SELECT indexname
192+
FROM pg_indexes
193+
WHERE schemaname = $1 AND tablename = $2 AND indexname = $3
194+
""",
195+
[schema_name, table_name, index_name]
196+
)
197+
198+
case result.rows do
199+
[[^index_name]] -> true
200+
_ -> false
201+
end
202+
end
203+
end

0 commit comments

Comments
 (0)