Fix adding extra unique constraint to columns that are already unique

[kvch]

Previously, pgroll failed to add unique constraint to columns that were already included in another unique constraint in some cases. The bug is triggered when the name of the column that is not yet unique comes earlier than the already unique column.

The error message we get:

Failed to complete migration: unable to execute complete operation: failed to create unique constraint from index "_pgroll_dup_fruits_name_key": pq: index "fruits_name_key" does not exist

The issue is during renaming phase. pgroll checked if the unique index is duplicated (thus, has to be renamed). Then pgroll checked if the column (being renamed) is included in the index. If it was part of the index, pgroll renamed the index to the final name. If that unique index happened to be a unique constraint pgroll transformed the index into a unique constraint. Good, yay.

However, if the unique index is duplicated, but the column (that was being renamed) was not part of the index definition, pgroll did not rename the index. If this unique index is a constraint, pgroll still tried to transform the index with the original name into a constraint. So we got the error message above.

Now this problem is fixed.

Related to #227

[andrew-farries]

Is it possible to add a test case for the problem that this fixes?

Based on the PR description I tried (using the migrations in this Gist) to add a UNIQUE constraint involving a column that was already UNIQUE and it worked as expected:

After completing both migrations the schema was:

+----------+------------------------+-----------------------------------------------------+----------+--------------+-------------+
| Column   | Type                   | Modifiers                                           | Storage  | Stats target | Description |
|----------+------------------------+-----------------------------------------------------+----------+--------------+-------------|
| id       | integer                |  not null default nextval('items_id_seq'::regclass) | plain    | <null>       | <null>      |
| name     | character varying(255) |  not null                                           | extended | <null>       | <null>      |
| producer | character varying(255) |  not null                                           | extended | <null>       | <null>      |
+----------+------------------------+-----------------------------------------------------+----------+--------------+-------------+
Indexes:
    "items_pkey" PRIMARY KEY, btree (id)
    "items_name_key" UNIQUE CONSTRAINT, btree (name)
    "unique_name_producer" UNIQUE CONSTRAINT, btree (name, producer)

This was using commit daba767

[kvch]

Haha, yes. I will redo the example. The test case I put together seemed too complex, but apparently I simplified to too much :D

[kvch]

I updated the migration file to trigger the bug. I had to reorder the columns in the constraint definition. I am adding more details to the pr description about the bug.

[andrew-farries]

Thanks for updating the example and PR description.

Is it worth also adding a testcase for it?

[kvch]

The example I added is kind of an e2e test for this. Or do you want to add a lower level test for rename.go?

[andrew-farries]

The example I added is kind of an e2e test for this. Or do you want to add a lower level test for rename.go?

I was wondering if it was possible/worthwhile to add a test to op_create_constraint_test.go, ie a regular ExecuteTests style test that covers this case.

The problem with using examples as e2e tests is that we have sometimes in the past gone back and rewritten/modified the example migrations.