Skip to content

Update database helpers to set the current_schema

Yannis Roussos requested to merge 256121-add-schema-to-migration-helpers-that-query-the-pg-catalog into master

What does this MR do?

With !41907 (merged), we have removed all assumptions about using the public schema as the schema that GitLab uses.

That means that a GitLab instance can run using any arbitrary schema or even using multiple different schemas for different setups.

We have not observed the latter use case, but it is an option that an instance could follow and that would mean that the same tables exist in multiple schemas. Or an instance may use a specific schema for GitLab and have test tables in public with the same names as ones defined in GitLab, like for example the very commonly used users, projects or issues.

All our Migration Helpers that directly query pg_class, pg_index, pg_constraint, etc currently do not set the schema when fetching information about database objects, which means that those helpers can match an object with the same relname in the wrong schema.

This MR adds the current schema to all queries that access the catalog and adds specs that test that our Migration helpers do not match the wrong object in the presence of multiple schemas.

Related issue: #256121 (closed)

Problem showcase

Using a simple test migration as an example:

  def up
    puts current_schema

    table_name = 'audit_events_part_5fc467ac26_202009'
    index_name = 'audit_events_part_5fc467ac26_202009_pkey'
    if postgres_exists_by_name?(table_name, index_name)
      puts "#{table_name}.#{index_name} exists!"
    else
      puts "#{table_name}.#{index_name} does NOT exist!"
    end

    table_name = 'audit_events_part_5fc467ac26_202009'
    constraint_name = 'check_492aaa021d'
    if check_constraint_exists?(table_name, constraint_name)
      puts "#{table_name} --> #{constraint_name} exists!"
    else
      puts "#{table_name} --> #{constraint_name} does NOT exist!"
    end
  end

In the example above, we are searching for two objects that exist in the gitlab_partitions_dynamicschema (which is not included in thesearch_path`)

Before, this update, it was matching both objects:

$ bundle exec rake db:migrate
== 20200928104232 TestCurrentSchema: migrating ================================
-- current_schema()
   -> 0.0008s

public
audit_events_part_5fc467ac26_202009.audit_events_part_5fc467ac26_202009_pkey exists!
audit_events_part_5fc467ac26_202009 --> check_492aaa021d exists!

== 20200928104232 TestCurrentSchema: migrated (0.0026s) =======================

After this update, our migration helpers correctly report that the index and the constraint can not be found:

$ bundle exec rake db:migrate
== 20200928104232 TestCurrentSchema: migrating ================================
-- current_schema()
   -> 0.0006s
public

-- current_schema()
   -> 0.0003s
audit_events_part_5fc467ac26_202009.audit_events_part_5fc467ac26_202009_pkey does NOT exist!

-- current_schema()
   -> 0.0002s
audit_events_part_5fc467ac26_202009 --> check_492aaa021d does NOT exist!
== 20200928104232 TestCurrentSchema: migrated (0.0037s) =======================

Similarly, with the updated specs, the existing version of the code would:

  1) Gitlab::Database::MigrationHelpers#index_exists_by_name? when an index exists for a table with the the same name in another schema returns true if an index exists
     Failure/Error:
       expect(model.index_exists_by_name?(:projects, 'test_index_on_name'))
         .to be_falsy
       expected: falsey value
            got: true

  2) Gitlab::Database::MigrationHelpers#check_constraint_exists? returns false if a constraint with the same name exists for the same table in another schema
     Failure/Error:
       expect(model.check_constraint_exists?(:projects, 'check_2'))
         .to be_falsy
       expected: falsey value
            got: true

  ... .... ...

With the updates, all specs pass.

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

Merge request reports