Skip to content

Commit

Permalink
Do not modify column type while modifying fk constraint
Browse files Browse the repository at this point in the history
The following syntax:

```elixir
alter table(:my_table) do
  modify :parent_id, references(:my_table, type: :binary_id, on_delete: :nilify_all, validate: false),
    from: {:binary_id, null: true}
end
```

Produces a SQL equivalent like this:

```sql
ALTER TABLE "my_table"
  ALTER COLUMN "parent_id" TYPE uuid,
  ADD CONSTRAINT "my_table_parent_id_fkey" FOREIGN KEY ("parent_id") REFERENCES "my_table"("id") ON DELETE SET NULL NOT VALID
```

The problem with the first directive, `ALTER COLUMN`, is that even the
type is the same, at least in PostgreSQL, is creating bunch of
`AccessExclusiveLock` (the most restrictive one) on each of the indexes
associated to that field, and the table itself, when that's not needed at all.

For example:

```console
test_dev=# BEGIN;
  LOCK TABLE "schema_migrations" IN SHARE UPDATE EXCLUSIVE MODE;
  ALTER TABLE "my_table" ALTER COLUMN "parent_id" TYPE uuid, ADD CONSTRAINT "my_table_parent_id_fkey" FOREIGN KEY ("parent_id") REFERENCES "my_table"("id") ON DELETE SET NULL NOT VALID;
  INSERT INTO "schema_migrations" ("version","inserted_at") VALUES ('20210718210952',NOW());
  SELECT locktype, relation::regclass, mode, transactionid AS tid, virtualtransaction AS vtid, pid, granted FROM pg_locks;
COMMIT;
BEGIN
LOCK TABLE
ALTER TABLE
INSERT 0 1
   locktype    |                     relation                        |           mode           |   tid   |  vtid   |  pid  | granted
---------------+-----------------------------------------------------+--------------------------+---------+---------+-------+---------
 relation      | schema_migrations                                   | RowExclusiveLock         |         | 4/14045 | 16813 | t
 virtualxid    |                                                     | ExclusiveLock            |         | 4/14045 | 16813 | t
 relation      | my_table_parent_id_ppppppp_id_index                 | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | pg_locks                                            | AccessShareLock          |         | 4/14045 | 16813 | t
 relation      | 134438                                              | AccessShareLock          |         | 4/14045 | 16813 | t
 relation      | 134438                                              | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | my_table_parent_id_rrrrr_index                      | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | my_table                                            | AccessShareLock          |         | 4/14045 | 16813 | t
 relation      | my_table                                            | ShareLock                |         | 4/14045 | 16813 | t
 relation      | my_table                                            | ShareRowExclusiveLock    |         | 4/14045 | 16813 | t
 relation      | my_table                                            | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | my_table_project_id_xxxx_index                      | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | 135159                                              | AccessShareLock          |         | 4/14045 | 16813 | t
 relation      | 135159                                              | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | 132875                                              | AccessShareLock          |         | 4/14045 | 16813 | t
 relation      | 132875                                              | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | 132507                                              | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | 132506                                              | AccessShareLock          |         | 4/14045 | 16813 | t
 relation      | 132506                                              | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | schema_migrations                                   | ShareUpdateExclusiveLock |         | 4/14045 | 16813 | t
 relation      | 132504                                              | AccessShareLock          |         | 4/14045 | 16813 | t
 relation      | 132504                                              | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | 135184                                              | AccessShareLock          |         | 4/14045 | 16813 | t
 relation      | 135184                                              | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | 132505                                              | AccessShareLock          |         | 4/14045 | 16813 | t
 relation      | 132505                                              | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | my_table_parent_id_ttttt_index                      | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 transactionid |                                                     | ExclusiveLock            | 2175345 | 4/14045 | 16813 | t
 relation      | my_table_parent_id_aaaaaaaaaaa_id_index             | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | 132503                                              | AccessShareLock          |         | 4/14045 | 16813 | t
 relation      | 132503                                              | AccessExclusiveLock      |         | 4/14045 | 16813 | t
 relation      | my_table_parent_id_gggg_index                       | AccessExclusiveLock      |         | 4/14045 | 16813 | t
(35 rows)

COMMIT
```

So, the proposal is to avoid setting the type when you pass a
`references` to `modify`, if there is any type incompatibility,
PostgreSQL, will let you know about it right away:

```
ERROR:  foreign key constraint "my_table_parent_id_fkey" cannot be implemented
DETAIL:  Key columns "parent_id" and "id" are of incompatible types: character varying and uuid.
```

Instead, the resulting SQL syntax should be something like:

```sql
ALTER TABLE "my_table"
  ADD CONSTRAINT "my_table_parent_id_fkey" FOREIGN KEY ("parent_id") REFERENCES "my_table"("id") ON DELETE SET NULL NOT VALID
```
That will produce more manageable lock types:

```console
   locktype    |     relation      |           mode           |   tid   |  vtid   |  pid  | granted
---------------+-------------------+--------------------------+---------+---------+-------+---------
 relation      | pg_locks          | AccessShareLock          |         | 4/14540 | 16849 | t
 relation      | schema_migrations | RowExclusiveLock         |         | 4/14540 | 16849 | t
 virtualxid    |                   | ExclusiveLock            |         | 4/14540 | 16849 | t
 transactionid |                   | ExclusiveLock            | 2175967 | 4/14540 | 16849 | t
 relation      | schema_migrations | ShareUpdateExclusiveLock |         | 4/14540 | 16849 | t
 relation      | my_table          | AccessShareLock          |         | 4/14540 | 16849 | t
 relation      | my_table          | ShareRowExclusiveLock    |         | 4/14540 | 16849 | t
(7 rows)
```
  • Loading branch information
milmazz committed Oct 17, 2024
1 parent 1f631fc commit 70a375f
Showing 1 changed file with 1 addition and 5 deletions.
6 changes: 1 addition & 5 deletions lib/ecto/adapters/postgres/connection.ex
Original file line number Diff line number Diff line change
Expand Up @@ -1548,11 +1548,7 @@ if Code.ensure_loaded?(Postgrex) do
defp column_change(table, {:modify, name, %Reference{} = ref, opts}) do
[
drop_reference_expr(opts[:from], table, name),
"ALTER COLUMN ",
quote_name(name),
" TYPE ",
reference_column_type(ref.type, opts),
", ADD ",
"ADD ",
reference_expr(ref, table, name),
modify_null(name, opts),
modify_default(name, ref.type, opts)
Expand Down

0 comments on commit 70a375f

Please sign in to comment.