Skip to content

Commit

Permalink
Make introspection FK column order deterministic (#624)
Browse files Browse the repository at this point in the history
<!-- The PR description should answer 2 (maybe 3) important questions:
-->


[ENG-1081](https://linear.app/hasura/issue/ENG-1081/postgres-foreign-key-constaints-are-wrong)

### What

<!-- What is this PR trying to accomplish (and why, if it's not
obvious)? -->

Order of foreign key columns is currently not deterministic.
This is a problem for composite foreign keys, where we independently
build two lists: the local columns and the foreign colums they map to.

The relevant introspection code follows:

Constrained (left-hand side) columns:
```sql
        SELECT
          c_unnest.constraint_id,
          array_agg(col.column_name) as key_columns
        FROM
          (
            SELECT
              c.oid as constraint_id,
              c.conrelid as relation_id,
              unnest(c.conkey) as column_number
            FROM
              pg_catalog.pg_constraint as c
          ) AS c_unnest
        INNER JOIN
          columns col
          USING (relation_id, column_number)
        GROUP BY c_unnest.constraint_id
```
Foreign (right-hand side) columns

```sql
        SELECT
          c_unnest.constraint_id,
          array_agg(col.column_name) as referenced_columns
        FROM
          (
            SELECT
              c.oid as constraint_id,
              c.confrelid as relation_id,
              unnest(c.confkey) as column_number
            FROM
              pg_catalog.pg_constraint as c
          ) AS c_unnest
        INNER JOIN
          columns col
          USING (relation_id, column_number)
        GROUP BY c_unnest.constraint_id
```

The above code attempts to fetch a list of columns names for both
left-hand and right-hand sides. Given a foreign key like so:

```sql
FOREIGN KEY (a, b) REFERENCES public.table(af, bf),
```
We want an array of the constrained columns:

| constraint_id | key_columns |
|--------------|----------------|
| 1                  | ['a','b']           |

And an array of the foreign columns:

| constraint_id | referenced_columns |
|--------------|----------------|
| 1                  | ['af','bf']           |

### The problem

`pg_catalog.pg_constraint.conkey` is an array of integers referencing
constrained columns
`pg_catalog.pg_constraint.conkey` is an array of integers referencing
foreign columns

The item at each index in each array correspond to the item in the same
index in the other array

We try to map the arrays of integers, to arrays of string column names

We need this behavior to be deterministic, so that after the mapping the
indices in each array still correspond to the same index in the other
array.

However, `unnest` is not guaranteed to preserve order. And neither is
`array_agg`

<!-- Consider: do we need to add a changelog entry? -->

### How

<!-- How is it trying to accomplish it (what are the implementation
steps)? -->

We use `UNNEST` as a table function, so we can use it `WITH ORDINALITY`.
This gives us an index we can use to make sure the ordering is
deterministic.

To ensure we keep that order, we add an `ORDER BY` clause inside
`array_agg`.

Note that using a subquery with an ordered result (via a normal `ORDER
BY` clause) before `array_agg` would not work!

```sql
        SELECT c.oid as constraint_id,
            array_agg(
                col.column_name
                ORDER BY k.index
            ) as key_columns
        FROM pg_catalog.pg_constraint as c
            CROSS JOIN UNNEST(c.conkey) WITH ORDINALITY k(column_number, index)
            INNER JOIN columns col ON c.conrelid = col.relation_id
            AND k.column_number = col.column_number
        GROUP BY c.oid
```
  • Loading branch information
BenoitRanque authored Oct 4, 2024
1 parent 7b969a3 commit 2579844
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 96 deletions.
2 changes: 2 additions & 0 deletions changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@

### Fixed

- Make introspection FK column order deterministic, preventing incorrect composite key column mapping

## [v1.1.1] - 2024-08-22

### Added
Expand Down
52 changes: 20 additions & 32 deletions crates/configuration/src/version3/version3.sql
Original file line number Diff line number Diff line change
Expand Up @@ -1142,41 +1142,29 @@ WITH
-- array.
constraint_columns AS
(
SELECT
c_unnest.constraint_id,
array_agg(col.column_name) as key_columns
FROM
(
SELECT
c.oid as constraint_id,
c.conrelid as relation_id,
unnest(c.conkey) as column_number
FROM
pg_catalog.pg_constraint as c
) AS c_unnest
INNER JOIN
columns col
USING (relation_id, column_number)
GROUP BY c_unnest.constraint_id
SELECT c.oid as constraint_id,
array_agg(
col.column_name
ORDER BY k.index
) as key_columns
FROM pg_catalog.pg_constraint as c
CROSS JOIN UNNEST(c.conkey) WITH ORDINALITY k(column_number, index)
INNER JOIN columns col ON c.conrelid = col.relation_id
AND k.column_number = col.column_number
GROUP BY c.oid
),
constraint_referenced_columns AS
(
SELECT
c_unnest.constraint_id,
array_agg(col.column_name) as referenced_columns
FROM
(
SELECT
c.oid as constraint_id,
c.confrelid as relation_id,
unnest(c.confkey) as column_number
FROM
pg_catalog.pg_constraint as c
) AS c_unnest
INNER JOIN
columns col
USING (relation_id, column_number)
GROUP BY c_unnest.constraint_id
SELECT c.oid as constraint_id,
array_agg(
col.column_name
ORDER BY k.index
) as referenced_columns
FROM pg_catalog.pg_constraint as c
CROSS JOIN UNNEST(c.confkey) WITH ORDINALITY k(column_number, index)
INNER JOIN columns col ON c.confrelid = col.relation_id
AND k.column_number = col.column_number
GROUP BY c.oid
)
SELECT
c.oid as constraint_id,
Expand Down
52 changes: 20 additions & 32 deletions crates/configuration/src/version4/introspection.sql
Original file line number Diff line number Diff line change
Expand Up @@ -1346,41 +1346,29 @@ WITH
-- array.
constraint_columns AS
(
SELECT
c_unnest.constraint_id,
array_agg(col.column_name) as key_columns
FROM
(
SELECT
c.oid as constraint_id,
c.conrelid as relation_id,
unnest(c.conkey) as column_number
FROM
pg_catalog.pg_constraint as c
) AS c_unnest
INNER JOIN
columns col
USING (relation_id, column_number)
GROUP BY c_unnest.constraint_id
SELECT c.oid as constraint_id,
array_agg(
col.column_name
ORDER BY k.index
) as key_columns
FROM pg_catalog.pg_constraint as c
CROSS JOIN UNNEST(c.conkey) WITH ORDINALITY k(column_number, index)
INNER JOIN columns col ON c.conrelid = col.relation_id
AND k.column_number = col.column_number
GROUP BY c.oid
),
constraint_referenced_columns AS
(
SELECT
c_unnest.constraint_id,
array_agg(col.column_name) as referenced_columns
FROM
(
SELECT
c.oid as constraint_id,
c.confrelid as relation_id,
unnest(c.confkey) as column_number
FROM
pg_catalog.pg_constraint as c
) AS c_unnest
INNER JOIN
columns col
USING (relation_id, column_number)
GROUP BY c_unnest.constraint_id
SELECT c.oid as constraint_id,
array_agg(
col.column_name
ORDER BY k.index
) as referenced_columns
FROM pg_catalog.pg_constraint as c
CROSS JOIN UNNEST(c.confkey) WITH ORDINALITY k(column_number, index)
INNER JOIN columns col ON c.confrelid = col.relation_id
AND k.column_number = col.column_number
GROUP BY c.oid
)
SELECT
c.oid as constraint_id,
Expand Down
52 changes: 20 additions & 32 deletions crates/configuration/src/version5/introspection.sql
Original file line number Diff line number Diff line change
Expand Up @@ -1346,41 +1346,29 @@ WITH
-- array.
constraint_columns AS
(
SELECT
c_unnest.constraint_id,
array_agg(col.column_name) as key_columns
FROM
(
SELECT
c.oid as constraint_id,
c.conrelid as relation_id,
unnest(c.conkey) as column_number
FROM
pg_catalog.pg_constraint as c
) AS c_unnest
INNER JOIN
columns col
USING (relation_id, column_number)
GROUP BY c_unnest.constraint_id
SELECT c.oid as constraint_id,
array_agg(
col.column_name
ORDER BY k.index
) as key_columns
FROM pg_catalog.pg_constraint as c
CROSS JOIN UNNEST(c.conkey) WITH ORDINALITY k(column_number, index)
INNER JOIN columns col ON c.conrelid = col.relation_id
AND k.column_number = col.column_number
GROUP BY c.oid
),
constraint_referenced_columns AS
(
SELECT
c_unnest.constraint_id,
array_agg(col.column_name) as referenced_columns
FROM
(
SELECT
c.oid as constraint_id,
c.confrelid as relation_id,
unnest(c.confkey) as column_number
FROM
pg_catalog.pg_constraint as c
) AS c_unnest
INNER JOIN
columns col
USING (relation_id, column_number)
GROUP BY c_unnest.constraint_id
SELECT c.oid as constraint_id,
array_agg(
col.column_name
ORDER BY k.index
) as referenced_columns
FROM pg_catalog.pg_constraint as c
CROSS JOIN UNNEST(c.confkey) WITH ORDINALITY k(column_number, index)
INNER JOIN columns col ON c.confrelid = col.relation_id
AND k.column_number = col.column_number
GROUP BY c.oid
)
SELECT
c.oid as constraint_id,
Expand Down

0 comments on commit 2579844

Please sign in to comment.