Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce index_dropped check #44

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Introduce index_dropped check #44

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d808a9
Introduce index_dropped check
kamilkowalski Dec 11, 2024
e450ba4
Add README
kamilkowalski Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 51 additions & 4 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ Potentially dangerous operations:
- [Renaming a column](#renaming-a-column)
- [Renaming a table](#renaming-a-table)
- [Setting NOT NULL on an existing column](#setting-not-null-on-an-existing-column)
- [Dropping an index](#dropping-an-index)

Postgres-specific checks:

Expand Down Expand Up @@ -257,7 +258,7 @@ To avoid a potentially lengthy update operation, particularly if you intend to f
1. insert the correct values using `UPDATE` query
1. only then add any desired default

Also creating a new table with column with volatile default is safe, because it does not contain any records.
Also creating a new table with column with volatile default is safe, because it does not contain any records.

---

Expand Down Expand Up @@ -369,8 +370,6 @@ Rename the field in the schema only, and configure it to point to the database c

```elixir
defmodule Cookbook.Recipe do
use Ecto.Schema

schema "recipes" do
field :author, :string
field :preparation_minutes, :integer, source: :prep_min
Expand Down Expand Up @@ -565,6 +564,53 @@ end

If your constraint fails, then you should consider backfilling data first to cover the gaps in your desired data integrity, then revisit validating the constraint.

### Dropping an index

Dropping an index can lead to performance issues if queries rely on that index. This is especially
risky when dropping and recreating indexes in the same deployment, as there will be a period where
no index exists.

**BAD ❌**

```elixir
def change do
drop index("recipes", [:user_id])
# During the time between dropping the old index and creating the new one, queries that used the
# original index will fall back to sequential scans, which can severely impact performance on
# large tables.
create index("recipes", [:user_id, :published_at])
end
```

**GOOD ✅**

Create the new index first, verify it's being used, then drop the old one. With Postgres, use
concurrent index creation to avoid blocking reads:

```elixir
# First migration
@disable_ddl_transaction true
@disable_migration_lock true

def change do
create index("recipes", [:user_id, :published_at], concurrently: true)
end
```

After deploying this migration, verify that your queries are actually using the new index. In
Postgres, you can use `EXPLAIN ANALYZE` to check the query plan and confirm the new index is being
used as expected.

```elixir
# Second migration (after confirming index usage)
def change do
drop index("recipes", [:user_id])
end
```

This ensures that there is always an index available for queries to use. The old index can be safely
dropped once the new index is fully built and available.

---

### Executing SQL directly
Expand Down Expand Up @@ -599,7 +645,7 @@ end

**GOOD ✅**

With Postgres, instead create the index concurrently which does not block reads. You will need to disable the database transactions to use `CONCURRENTLY`, and since Ecto obtains migration locks through database transactions this also implies that competing nodes may attempt to try to run the same migration (eg, in a multi-node Kubernetes environment that runs migrations before startup). Therefore, some nodes will fail startup for a variety of reasons.
With Postgres, instead create the index concurrently which does not block reads. You will need to disable the database transactions to use `CONCURRENTLY`, and since Ecto obtains migration locks through database transactions this also implies that competing nodes may attempt to try to run the same migration (eg, in a multi-node Kubernetes environment that runs migrations before startup). Therefore, some nodes will fail startup for a variety of reasons.

```elixir
@disable_ddl_transaction true
Expand Down Expand Up @@ -788,6 +834,7 @@ Possible operation types are:
* `column_volatile_default`
* `index_concurrently_without_disable_ddl_transaction`
* `index_concurrently_without_disable_migration_lock`
* `index_dropped`
* `index_not_concurrently`
* `json_column_added`
* `many_columns_index`
Expand Down
10 changes: 9 additions & 1 deletion lib/ast_parser.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ defmodule ExcellentMigrations.AstParser do
detect_not_null_added(code_part) ++
detect_check_constraint(code_part) ++
detect_records_modified(code_part) ++
detect_json_column_added(code_part)
detect_json_column_added(code_part) ++
detect_index_dropped(code_part)
end

defp detect_index_not_concurrently({fun_name, location, [{operation, _, [_, _]}]})
Expand Down Expand Up @@ -214,4 +215,11 @@ defmodule ExcellentMigrations.AstParser do
end

defp detect_safety_assured(_), do: []

defp detect_index_dropped({fun_name, location, [{operation, _, _}]})
when fun_name in [:drop, :drop_if_exists] and operation in @index_types do
[{:index_dropped, Keyword.get(location, :line)}]
end

defp detect_index_dropped(_), do: []
end
1 change: 1 addition & 0 deletions lib/dangers_detector.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ defmodule ExcellentMigrations.DangersDetector do
| :column_volatile_default
| :index_concurrently_without_disable_ddl_transaction
| :index_concurrently_without_disable_migration_lock
| :index_dropped
| :index_not_concurrently
| :json_column_added
| :many_columns_index
Expand Down
1 change: 1 addition & 0 deletions lib/runner.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ defmodule ExcellentMigrations.Runner do
| :column_volatile_default
| :index_concurrently_without_disable_ddl_transaction
| :index_concurrently_without_disable_migration_lock
| :index_dropped
| :index_not_concurrently
| :json_column_added
| :many_columns_index
Expand Down
12 changes: 12 additions & 0 deletions test/ast_parser_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,18 @@ defmodule ExcellentMigrations.AstParserTest do
assert [table_dropped: 1] == AstParser.parse(ast4)
end

test "detects index dropped" do
ast1 = string_to_ast("drop index(:recipes, [:cuisine], concurrently: true)")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose also testing version without any options, because it's a different arity that produces slightly different AST structure.

Suggested change
ast1 = string_to_ast("drop index(:recipes, [:cuisine], concurrently: true)")
ast = string_to_ast("drop index(:recipes, [:cuisine])")
ast1 = string_to_ast("drop index(:recipes, [:cuisine], concurrently: true)")

ast2 = string_to_ast("drop_if_exists index(:recipes, [:cuisine], concurrently: true)")
ast3 = string_to_ast("drop unique_index(:recipes, [:cuisine], concurrently: true)")
ast4 = string_to_ast("drop_if_exists unique_index(:recipes, [:cuisine], concurrently: true)")

assert [index_dropped: 1] == AstParser.parse(ast1)
assert [index_dropped: 1] == AstParser.parse(ast2)
assert [index_dropped: 1] == AstParser.parse(ast3)
assert [index_dropped: 1] == AstParser.parse(ast4)
end

defp safety_assured_ast do
string_to_ast("""
@safety_assured [:index_not_concurrently]
Expand Down
18 changes: 0 additions & 18 deletions test/example_migrations/20220726000151_create_index_concurrently_valid.exs
View file
Open in desktop

This file was deleted.

19 changes: 11 additions & 8 deletions test/example_migrations/20220726010151_create_index_concurrently_invalid.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
defmodule ExcellentMigrations.CreateIndexConcurrentlyInvalid do
@disable_migration_lock true
@disable_ddl_transaction true

def up do
create(index(:dumplings, [:dough], concurrently: true))
create_if_not_exists(index(:dumplings, [:dough], concurrently: true))
create(unique_index(:dumplings, [:dough], concurrently: true))
create_if_not_exists(unique_index(:dumplings, [:dough], concurrently: true))
create(index(:dumplings, [:dough], concurrently: false))
create_if_not_exists(index(:dumplings, [:dough], concurrently: false))
create(unique_index(:dumplings, [:dough], concurrently: false))
create_if_not_exists(unique_index(:dumplings, [:dough], concurrently: false))
end

def down do
drop(index(:dumplings, [:dough], concurrently: true))
drop_if_exists(index(:dumplings, [:dough], concurrently: true))
drop(unique_index(:dumplings, [:dough], concurrently: true))
drop_if_exists(unique_index(:dumplings, [:dough], concurrently: true))
drop(index(:dumplings, [:dough], concurrently: false))
drop_if_exists(index(:dumplings, [:dough], concurrently: false))
drop(unique_index(:dumplings, [:dough], concurrently: false))
drop_if_exists(unique_index(:dumplings, [:dough], concurrently: false))
end
end
140 changes: 130 additions & 10 deletions test/runner_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,6 @@ defmodule ExcellentMigrations.RunnerTest do
use ExUnit.Case
alias ExcellentMigrations.Runner

test "it should be valid migration files" do
file_paths = [
"test/example_migrations/20220726000151_create_index_concurrently_valid.exs"
]

assert :safe == Runner.check_migrations(migrations_paths: file_paths)
end

test "generates warning messages for migration files" do
file_paths = [
"test/example_migrations/20191026103001_create_table_and_index.exs",
Expand Down Expand Up @@ -85,11 +77,21 @@ defmodule ExcellentMigrations.RunnerTest do
path: "test/example_migrations/20220725111000_create_index.exs",
type: :index_not_concurrently
},
%{
line: 8,
path: "test/example_migrations/20220725111000_create_index.exs",
type: :index_dropped
},
%{
line: 9,
path: "test/example_migrations/20220725111000_create_index.exs",
type: :index_not_concurrently
},
%{
line: 9,
path: "test/example_migrations/20220725111000_create_index.exs",
type: :index_dropped
},
%{
line: 3,
path: "test/example_migrations/20220725111501_create_unique_index.exs",
Expand All @@ -105,29 +107,147 @@ defmodule ExcellentMigrations.RunnerTest do
path: "test/example_migrations/20220725111501_create_unique_index.exs",
type: :index_not_concurrently
},
%{
line: 8,
path: "test/example_migrations/20220725111501_create_unique_index.exs",
type: :index_dropped
},
%{
line: 9,
path: "test/example_migrations/20220725111501_create_unique_index.exs",
type: :index_not_concurrently
},
%{
line: 9,
path: "test/example_migrations/20220725111501_create_unique_index.exs",
type: :index_dropped
},
%{
line: 6,
path:
"test/example_migrations/20220726010151_create_index_concurrently_invalid.exs",
type: :index_not_concurrently
},
%{
line: 7,
path:
"test/example_migrations/20220726010151_create_index_concurrently_invalid.exs",
type: :index_not_concurrently
},
%{
line: 8,
path:
"test/example_migrations/20220726010151_create_index_concurrently_invalid.exs",
type: :index_not_concurrently
},
%{
line: 9,
path:
"test/example_migrations/20220726010151_create_index_concurrently_invalid.exs",
type: :index_not_concurrently
},
%{
line: 13,
path:
"test/example_migrations/20220726010151_create_index_concurrently_invalid.exs",
type: :index_concurrently_without_disable_migration_lock
type: :index_not_concurrently
},
%{
line: 13,
path:
"test/example_migrations/20220726010151_create_index_concurrently_invalid.exs",
type: :index_concurrently_without_disable_ddl_transaction
type: :index_dropped
},
%{
line: 14,
path:
"test/example_migrations/20220726010151_create_index_concurrently_invalid.exs",
type: :index_not_concurrently
},
%{
line: 14,
path:
"test/example_migrations/20220726010151_create_index_concurrently_invalid.exs",
type: :index_dropped
},
%{
line: 15,
path:
"test/example_migrations/20220726010151_create_index_concurrently_invalid.exs",
type: :index_not_concurrently
},
%{
line: 15,
path:
"test/example_migrations/20220726010151_create_index_concurrently_invalid.exs",
type: :index_dropped
},
%{
line: 16,
path:
"test/example_migrations/20220726010151_create_index_concurrently_invalid.exs",
type: :index_not_concurrently
},
%{
line: 16,
path:
"test/example_migrations/20220726010151_create_index_concurrently_invalid.exs",
type: :index_dropped
},
%{
line: 12,
path:
"test/example_migrations/20220804010152_create_index_concurrently_without_disable_ddl_transaction.exs",
type: :index_dropped
},
%{
line: 13,
path:
"test/example_migrations/20220804010152_create_index_concurrently_without_disable_ddl_transaction.exs",
type: :index_dropped
},
%{
line: 14,
path:
"test/example_migrations/20220804010152_create_index_concurrently_without_disable_ddl_transaction.exs",
type: :index_dropped
},
%{
line: 15,
path:
"test/example_migrations/20220804010152_create_index_concurrently_without_disable_ddl_transaction.exs",
type: :index_dropped
},
%{
line: 15,
path:
"test/example_migrations/20220804010152_create_index_concurrently_without_disable_ddl_transaction.exs",
type: :index_concurrently_without_disable_ddl_transaction
},
%{
line: 12,
path:
"test/example_migrations/20220804010153_create_index_concurrently_without_disable_migration_lock.exs",
type: :index_dropped
},
%{
line: 13,
path:
"test/example_migrations/20220804010153_create_index_concurrently_without_disable_migration_lock.exs",
type: :index_dropped
},
%{
line: 14,
path:
"test/example_migrations/20220804010153_create_index_concurrently_without_disable_migration_lock.exs",
type: :index_dropped
},
%{
line: 15,
path:
"test/example_migrations/20220804010153_create_index_concurrently_without_disable_migration_lock.exs",
type: :index_dropped
},
%{
line: 15,
path:
Expand Down