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

Jump to bottom

UNIQUE constraints are dropped by migrations in non-public schemas #273

Closed
andrew-farries opened this issue Feb 5, 2024 · 0 comments · Fixed by #279
Closed

UNIQUE constraints are dropped by migrations in non-public schemas #273

andrew-farries opened this issue Feb 5, 2024 · 0 comments · Fixed by #279
Assignees
@andrew-farries
Labels
bug Something isn't working

Comments

@andrew-farries
Copy link
Collaborator

When duplicating columns for backfilling, UNIQUE constraints are dropped when the migration is applied in a schema other than public.

To reproduce:

Run the series of migrations in this gist in a schema other than public, eg:

go run . start --complete --schema foo 01_create_table.json
go run . start --complete --schema foo 02_add_column.json
go run . start --complete --schema foo 03_drop_not_null.json
go run . start --complete --schema foo 04_set_not_null.json

The final schema lacks the unique constraint on the two column:

+----------------+--------------------------+-------------------------+----------+--------------+-------------+
| Column         | Type                     | Modifiers               | Storage  | Stats target | Description |
|----------------+--------------------------+-------------------------+----------+--------------+-------------|
| xata_id        | text                     |  not null               | extended | <null>       | <null>      |
| xata_version   | integer                  |  not null default 0     | plain    | <null>       | <null>      |
| xata_createdat | timestamp with time zone |  not null default now() | plain    | <null>       | <null>      |
| xata_updatedat | timestamp with time zone |  not null default now() | plain    | <null>       | <null>      |
| two            | integer                  |  not null               | plain    | <null>       | <null>      |
+----------------+--------------------------+-------------------------+----------+--------------+-------------+
Indexes:
    "_pgroll_new_tester_pkey" PRIMARY KEY, btree (xata_id)

Preservation of UNIQUE constraints was implemented as part of #227, but missed this specific case of UNIQUE constraints in migrations in non-public schema.
@andrew-farries andrew-farries added the bug Something isn't working label Feb 5, 2024
@andrew-farries andrew-farries self-assigned this Feb 5, 2024
@andrew-farries andrew-farries mentioned this issue Feb 5, 2024
Merged
andrew-farries added a commit that referenced this issue Feb 5, 2024
@andrew-farries
Parameterize schema in which migration tests run via `PGROLL_TEST_SCH…
c95bc78 
…EMA` env var (#276)

Parameterize the migration tests so that the schema in which migrations
are applied is taken from the `PGROLL_TEST_SCHEMA` environment variable,
or `public` if unset.

#273 highlights an area in which there is a lack of test coverage:
migration tests apply migrations only in the `public` schema. #273 is an
error that is only reproducible when running migrations in a
non-`public` schema and there are currently also other problems with
migration validation in non-`public` schema.

This PR makes repetitive changes to all tests:
* Update the `afterStart`, `afterComplete` and `afterRollback` hooks to
take a `schema string` parameter.
* Update all hard-coded occurences of `"public"` in those hooks with the
`schema` parameter.

Later PRs will fix the issues with running migrations in non-`public`
schema and run non-`public` migration tests in CI. For now, to run
migration tests in a non-`public` schema run:

```
PGROLL_TEST_SCHEMA=foo go test ./...
```

Part of #273
@andrew-farries andrew-farries mentioned this issue Feb 5, 2024
Merged
andrew-farries added a commit that referenced this issue Feb 6, 2024
@andrew-farries
Strengthen assertions for tests that check preservation of UNIQUE c…
5686ddc 
…onstraints (#277)

Strengthen the tests that check for preservation of `UNIQUE` constraints
when a column is duplicated so that they can check for the failure case
in #273.

It's not enough to test that the column does not accept duplicate values
after the migration completes. Both a unique index and a unique
constraint will have that effect but we need to ensure that the
**constraint** is present on the table once the migration completes.

Duplicating a `UNIQUE` constraint is a two-step process, first creating
a `UNIQUE` index on migration start and then upgrading the index to a
constraint on migration completion. #273 occurs when this upgrade fails
and the column is left with the unique index but not the constraint.
Subsequent migrations will then fail to duplicate the non-existent
unique constraint.

Part of #273
This was referenced Feb 6, 2024
Merged
Merged
andrew-farries added a commit that referenced this issue Feb 7, 2024
@andrew-farries
Strip schema from index names in internal schema representation (#278)
e906da7 
Remove the schema component from index names in the internal schema
representation.

A `pgroll` migration is always run in the context of a a specific schema
so the extra qualification is redundant.

Removing the schema component from the index names makes it easy to
identify duplicated indexes by name when temporary columns and
associated attributes are renamed on migration completion.

Tests for this change are already in place (#276, #277). As of this PR,
migration tests run in a non-public schema will pass:

```bash
PGROLL_TEST_SCHEMA=foo go test ./...
```

Part of #273
andrew-farries added a commit that referenced this issue Feb 7, 2024
@andrew-farries
Run migration tests in a non-public schema as part of CI (#279)
889946b 
Add another dimension to the test matrix so that migration tests are run
in both the `public` and a non-`public` schema.

#276 made it possible to run
migration tests in schema other than `public`. Doing so highlighted some
issues that are fixed by #278.

Fixes #273.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@andrew-farries andrew-farries

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Run migration tests in a non-public schema as part of CI xataio/pgroll
1 participant
@andrew-farries