Skip to content

Add migration helpers for fetching and copying check constraints - Take 2

Yannis Roussos requested to merge 247489-update-create-column-from-to-also-copy-constraints-take-2 into master

Root Cause Analysis of issues encountered in original MR

This MR fixes issues encountered after !44025 (merged) was merged, which resulted to reverting all the introduced changes (!44382 (merged)).

We have identified two different problems:

  1. The column pg_catalog.pg_constraint.consrc, which is used by the newly introduced check_constraints_for migration helper, has been removed in PostgreSQL 12 (PG12 vs PG11).

    On MR pipelines, we run tests for PostgreSQL 11, but not for PG12, so we never caught that error on the pipelines.

    The error was caught after it was merged to master as we run PG12 tests for master.

    Corrective action: We switch to use pg_get_constraintdef() that works up until the current PG13 version.

  2. The original MR also fixed create_column_from to also copy constraints from the original column, by using copy_check_constraints, which in turn uses check_constraints_for.

    All the affected specs of spec/lib/gitlab/database/migration_helpers_spec.rb were properly updated.

    But there are additional specs for background migrations under spec/migrations/ which were also affected by the change.

    The test job(s) for those specs also never run in the MR's pipeline.

    The reason for that is that lib/gitlab/database/migration_helpers.rb was not included in the rspec migration rules, so changes in our migration helpers never kick off the migration related specs.

    Corrective actions:

    1. With !44384 (merged), we have updated our rspec migration rules, so that the migration specs also run when changes in our database libraries are detected.
    2. The affected background migration specs under spec/migrations/ run without issues after the latest update.

Additionally, in this MR we also added:

  • Update copy_check_constraints() to recognize NOT VALID constraints and set the constraint on the new column in the same manner
  • Update copy_check_constraints() to parse and normalize check constraint definitions of various types and flavors
  • Update specs with the new format for check constraint definitions returned by pg_get_constraintdef()
  • Update spec for migration helpers to also check for the case when NOT VALID constraints are used with copy_check_constraints

Original MR description (with all examples run again in the final updated version of the code):

What does this MR do?

This MR adds migration helpers for copying check constraints

  • Adds check_constraints_for(table, column, schema: nil) private method to Gitlab::Database::MigrationHelpers.

    Returns all the check constraints defined for a column.

  • Adds the copy_check_constraints(table, old, new, schema: nil) migration helper.

    It copies all constraints defined in column old to column new.

  • Updates the create_column_from migration helper to also copy all existing check constraints to the new column.

  • Updates the specs for all helpers that use create_column_from

  • Adds new specs for copy_check_constraints

Related Issue: #247489 (closed)

What does this MR does not do?

It only focusses on check constraints and adding support for them to create_column_from

It does not address the existing issue with rolling back a create_column_from that converts NOT NULL constraints to column IS NOT NULL check constraints.

That will be discussed and dealt with in #259699 (closed)

Proof of concept

Given the following test migration:

class TestCopyCheckConstraints < ActiveRecord::Migration[6.0]
  include Gitlab::Database::MigrationHelpers

  DOWNTIME = false
  disable_ddl_transaction!

  def up
    create_table :test_copy_check_constraints, if_not_exists: true, id: false do |t|
      t.text :aaaa
      t.text :aaaa2
      t.text :bbbb
      t.text :bbbb2
      t.text :bbbb3
      t.bigint :count, null: false
      t.bigint :count2
    end

    add_not_null_constraint :test_copy_check_constraints, :bbbb
    add_text_limit :test_copy_check_constraints, :bbbb, 255

    add_not_null_constraint :test_copy_check_constraints, :bbbb3
    add_text_limit :test_copy_check_constraints, :bbbb3, 255

    add_check_constraint :test_copy_check_constraints, 'count > 0', 'check_count_positive'

    # Nothing should happen
    copy_check_constraints :test_copy_check_constraints, :aaaa, :aaaa2

    # The two constraints should be copied
    copy_check_constraints :test_copy_check_constraints, :bbbb, :bbbb2

    # No additional constraints should be added to bbbb3 as the ones
    # copied from :bbb should try to use the same names
    copy_check_constraints :test_copy_check_constraints, :bbbb, :bbbb3

    # The custom check constraint should be copied and use an auto generated name
    # `null:false` should not be copied as a check constraint
    copy_check_constraints :test_copy_check_constraints, :count, :count2

    # The custom check constraint should be copied and use an auto generated name
    # `null:false` should be converted to a check constraint
    create_column_from :test_copy_check_constraints, :count, :new_count
  end

  def down
    drop_table :test_copy_check_constraints
  end
end

When the migration runs, the correct constraints are added to the table (diff annotated for better reading experience)

$ bundle exec rake db:migrate
 ... ... ...

$ git diff db/structure.sql
... ...
+CREATE TABLE test_copy_check_constraints (
+    aaaa text,
+    aaaa2 text,
+    bbbb text,
+    bbbb2 text,
+    bbbb3 text,
+    count bigint NOT NULL,
+    count2 bigint,
+    new_count bigint,

# Manually added constraints for bbbb
+    CONSTRAINT check_d7d49d475d CHECK ((bbbb IS NOT NULL))
+    CONSTRAINT check_48560e521e CHECK ((char_length(bbbb) <= 255)),

# All constraints were properly copied from bbbb to bbbb2
+    CONSTRAINT check_9dcd761e37 CHECK ((bbbb2 IS NOT NULL)),
+    CONSTRAINT check_17c59b658a CHECK ((char_length(bbbb2) <= 255)),

# No duplicate constraints added for bbbb3 - only the manually added ones are there
+    CONSTRAINT check_46e55d045a CHECK ((bbbb3 IS NOT NULL)),
+    CONSTRAINT check_395c951858 CHECK ((char_length(bbbb3) <= 255)),

# Manually added constraint for count
+    CONSTRAINT check_count_positive CHECK ((count > 0)),

# Custom check constraint was copied to count2
+    CONSTRAINT check_1fc2397501 CHECK ((count2 > 0)),

# new_count was created from count with only difference the switch of the NOT NULL
+    CONSTRAINT check_48b708485f CHECK ((new_count IS NOT NULL)),
+    CONSTRAINT check_ce41781033 CHECK ((new_count > 0)),

+);

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

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
Edited by Yannis Roussos

Merge request reports