Fix duplicates in create_table_with_constraints (!56611) · Merge requests · GitLab.org / GitLab

What does this MR do?

Fix the create_table_with_constraints helper to not attempt to create duplicate constraints when the with_lock_retries wrapping the method body executes the internal create_table more than once.

For example, migration specs fail with a message like (see #324416 (closed)):

  PG::DuplicateObject: ERROR:  constraint "check_b634ca168d" for relation "external_approval_rules" already exists
# ./lib/gitlab/database/migration_helpers.rb:121:in `block in create_table_with_constraints'

This can be recreated locally when running a migration which creates a foreign key.

In a database session, start a transaction, and lock the reference tabled in the below migration with a lock table namespaces in share mode
Run the migration rails db:migrate:up VERSION=20201228110136
Watch for the with_lock_retries to retry the migration block multiple times
Rollback the transaction with the lock on namespaces

rails db:migrate:up VERSION=20201228110136
== 20201228110136 CreateIterationsCadence: migrating ==========================
-- create_table(:iterations_cadences, {})
-- quote_column_name(:title)
   -> 0.0000s
-- create_table(:iterations_cadences, {})
-- quote_column_name(:title)
   -> 0.0000s
-- create_table(:iterations_cadences, {})
-- quote_column_name(:title)
   -> 0.0000s
-- create_table(:iterations_cadences, {})
-- quote_column_name(:title)
   -> 0.0000s
-- create_table(:iterations_cadences, {})
-- quote_column_name(:title)
   -> 0.0000s
-- create_table(:iterations_cadences, {})
-- quote_column_name(:title)
   -> 0.0000s
-- create_table(:iterations_cadences, {})
-- quote_column_name(:title)
   -> 0.0000s
   -> 0.0078s
-- quote_table_name("check_fedff82d3b")
   -> 0.0000s
-- quote_table_name("check_fedff82d3b")
   -> 0.0000s
-- quote_table_name("check_fedff82d3b")
   -> 0.0000s
-- quote_table_name("check_fedff82d3b")
   -> 0.0000s
-- quote_table_name("check_fedff82d3b")
   -> 0.0000s
-- quote_table_name("check_fedff82d3b")
   -> 0.0000s
-- quote_table_name("check_fedff82d3b")
   -> 0.0000s
-- quote_table_name(:iterations_cadences)
   -> 0.0000s
-- execute("ALTER TABLE \"iterations_cadences\"\nADD CONSTRAINT \"check_fedff82d3b\" CHECK (char_length(\"title\") <= 255),\nADD CONSTRAINT \"check_fedff82d3b\" CHECK (char_length(\"title\") <= 255),\nADD CONSTRAINT \"check_fedff82d3b\" CHECK (char_length(\"title\") <= 255),\nADD CONSTRAINT \"check_fedff82d3b\" CHECK (char_length(\"title\") <= 255),\nADD CONSTRAINT \"check_fedff82d3b\" CHECK (char_length(\"title\") <= 255),\nADD CONSTRAINT \"check_fedff82d3b\" CHECK (char_length(\"title\") <= 255),\nADD CONSTRAINT \"check_fedff82d3b\" CHECK (char_length(\"title\") <= 255)\n")

There is one definition of the check constraint for each time with_lock_retries executed the inner block. After the behavior has been fixed, the output looks like:

rails db:migrate:up VERSION=20201228110136
== 20201228110136 CreateIterationsCadence: migrating ==========================
-- create_table(:iterations_cadences, {})
-- quote_column_name(:title)
   -> 0.0000s
-- create_table(:iterations_cadences, {})
-- quote_column_name(:title)
   -> 0.0000s
-- create_table(:iterations_cadences, {})
-- quote_column_name(:title)
   -> 0.0000s
-- create_table(:iterations_cadences, {})
-- quote_column_name(:title)
   -> 0.0000s
-- create_table(:iterations_cadences, {})
-- quote_column_name(:title)
   -> 0.0000s
-- create_table(:iterations_cadences, {})
-- quote_column_name(:title)
   -> 0.0000s
   -> 0.4345s
-- quote_table_name("check_fedff82d3b")
   -> 0.0000s
-- quote_table_name(:iterations_cadences)
   -> 0.0000s
-- execute("ALTER TABLE \"iterations_cadences\"\nADD CONSTRAINT \"check_fedff82d3b\" CHECK (char_length(\"title\") <= 255)\n")
   -> 0.0009s
== 20201228110136 CreateIterationsCadence: migrated (2.0057s) =================

Screenshots (strongly suggested)

Does this MR meet the acceptance criteria?

Conformity

📋 Does this MR need a changelog?
- I have included a changelog entry.
- I have not included a changelog entry because _____.
Documentation (if required)
Code review guidelines
Merge request performance guidelines
Style guides
Database guides
Separation of EE specific content

Availability and Testing

Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process.
Tested in all supported browsers
Informed Infrastructure department of a default or new setting change, if applicable per definition of done

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

Label as security and @ mention @gitlab-com/gl-security/appsec
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
Security reports checked/validated by a reviewer from the AppSec team

Fix duplicates in create_table_with_constraints