Skip to content
Snippets Groups Projects

Add FK from partitioned web_hook_logs to web_hooks (Part 3 of partitioned web_hook_logs migrations)

Merged Yannis Roussos requested to merge 323676-add-fk-to-partitioned-web-hook-logs into master
All threads resolved!

What does this MR do?

Related issue: #323676 (closed)

We have split !59190 (closed) to three MRs as there is considerable overhead to add each index to each partition of web_hook_logs. For more information and a full analysis, check !59190 (closed)

This is the third migration, in which we are adding a foreign key from the partitioned web_hook_logs (web_hook_logs_part_0c5294f417) to web_hooks.

We are also adding a new partitioning migration helper (add_concurrent_partitioned_foreign_key) that allows us to add foreign keys to partitioned tables with minimal locking and while overcoming PostgreSQL's restriction that does not allow adding NOT VALID foreign keys to partitioned tables.

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

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Yannis Roussos changed milestone to %13.11

    changed milestone to %13.11

  • Yannis Roussos changed title from Add FK from partitioned web_hook_logs to web_hooks to Add FK from partitioned web_hook_logs to web_hooks (Part 3 of partitioned web_hook_logs migrations)

    changed title from Add FK from partitioned web_hook_logs to web_hooks to Add FK from partitioned web_hook_logs to web_hooks (Part 3 of partitioned web_hook_logs migrations)

  • 1 Warning
    :warning:

    featureaddition and featureenhancement merge requests normally have a documentation change. Consider adding a documentation update or confirming the documentation plan with the Technical Writer counterpart.

    For more information, see:

    Mentioning @gitlab-data/engineers to notify the team about changes to the db/structure.sql file.

    Reviewer roulette

    Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.

    To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.

    To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.

    Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.

    Category Reviewer Maintainer
    backend Shreyas Agarwal (@shreyasagarwal) (UTC+5, 2 hours ahead of @iroussos) Kerri Miller (@kerrizor) (UTC-7, 10 hours behind @iroussos)
    database Alex Ives (@alexives) (UTC-5, 8 hours behind @iroussos) Steve Abrams (@sabrams) (UTC-6, 9 hours behind @iroussos)
    ~migration No reviewer available No maintainer available

    If needed, you can retry the danger-review job that generated this comment.

    Generated by :no_entry_sign: Danger

    Edited by 🤖 GitLab Bot 🤖
  • Yannis Roussos mentioned in merge request !59190 (closed)

    mentioned in merge request !59190 (closed)

  • Author Contributor

    I have started a gitlab-com-database-testing pipeline to test the FK addition - quick reference to the job

  • Author Contributor

    @abrandl this is the MR with the new helper for adding FKs to partitioned tables and the actual migration that will run. I have started a test pipeline, so let's see how it will work :-)

    Do you mind checking the helper and tell me if it looks good to you or whether you see any issues?

    Also, now that we are moving to PG12 we may want to strip the experimental helpers for adding FKs that reference partitioned tables. Let's think about it and we can remove them in a follow-up MR.

  • Yannis Roussos requested review from @abrandl

    requested review from @abrandl

  • Database migrations

    Migrations included in this change have been executed on gitlab.com data for testing purposes. For details, please see the migration testing pipeline (limited access). Note that this includes pending migrations from master.

    Migration Total runtime Result DB size change Note
    20210329102724 1.5 s :white_check_mark: +0.00 B (unrelated to this MR, but merged and pending on GitLab.com)
    20210409185531 1.1 s :white_check_mark: -616.27 MiB (unrelated to this MR, but merged and pending on GitLab.com)
    20210409185501 186.8 s :white_check_mark: +721.55 MiB (unrelated to this MR, but merged and pending on GitLab.com)
    20210407150240 1.4 s :white_check_mark: +8.00 KiB (unrelated to this MR, but merged and pending on GitLab.com)
    20210413132500 16.6 s :white_check_mark: +0.00 B (unrelated to this MR, but merged and pending on GitLab.com)
    20210413130011 1249.6 s :white_check_mark: +32.00 KiB
    20210412142223 17.2 s :white_check_mark: +19.74 MiB (unrelated to this MR, but merged and pending on GitLab.com)
    20210413123832 63527.1 s :white_check_mark: +13.19 GiB (unrelated to this MR, but merged and pending on GitLab.com)

    Migration: 20210413130011

    • Duration: 1249.6 s
    • Database size change: +32.00 KiB
    Query Calls Total Time Max Time Mean Time Rows
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202104 VALIDATE CONSTRAINT fk_rails_bb3355782d
    1 892986.3 ms 892986.3 ms 892986.3 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202103 VALIDATE CONSTRAINT fk_rails_bb3355782d
    1 173021.1 ms 173021.1 ms 173021.1 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202008 VALIDATE CONSTRAINT fk_rails_bb3355782d
    1 36172.7 ms 36172.7 ms 36172.7 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202012 VALIDATE CONSTRAINT fk_rails_bb3355782d
    1 32300.2 ms 32300.2 ms 32300.2 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202101 VALIDATE CONSTRAINT fk_rails_bb3355782d
    1 32122.9 ms 32122.9 ms 32122.9 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202102 VALIDATE CONSTRAINT fk_rails_bb3355782d
    1 28552.1 ms 28552.1 ms 28552.1 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202011 VALIDATE CONSTRAINT fk_rails_bb3355782d
    1 18308.3 ms 18308.3 ms 18308.3 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202010 VALIDATE CONSTRAINT fk_rails_bb3355782d
    1 16512.5 ms 16512.5 ms 16512.5 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202009 VALIDATE CONSTRAINT fk_rails_bb3355782d
    1 12755.1 ms 12755.1 ms 12755.1 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_000000 ADD CONSTRAINT fk_rails_bb3355782d FOREIGN KEY (web_hook_id) REFERENCES web_hooks (id) ON DELETE CASCADE NOT VALID
    1 115.2 ms 115.2 ms 115.2 ms 0
    ALTER TABLE "web_hook_logs_part_0c5294f417" ADD CONSTRAINT "fk_rails_bb3355782d" FOREIGN KEY ("web_hook_id") REFERENCES "web_hooks" ("id") ON DELETE CASCADE /*application:test*/
    1 41.6 ms 41.6 ms 41.6 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_000000 VALIDATE CONSTRAINT fk_rails_bb3355782d
    1 34.7 ms 34.7 ms 34.7 ms 0
    SELECT "postgres_partitions".*
      FROM "postgres_partitions"
      WHERE "postgres_partitions"."parent_identifier" = $1
      ORDER BY "postgres_partitions"."name" ASC /*application:test*/
    1 9.4 ms 9.4 ms 9.4 ms 17
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202109 VALIDATE CONSTRAINT fk_rails_bb3355782d
    1 8.2 ms 8.2 ms 8.2 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202108 ADD CONSTRAINT fk_rails_bb3355782d FOREIGN KEY (web_hook_id) REFERENCES web_hooks (id) ON DELETE CASCADE NOT VALID
    1 7.8 ms 7.8 ms 7.8 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202106 ADD CONSTRAINT fk_rails_bb3355782d FOREIGN KEY (web_hook_id) REFERENCES web_hooks (id) ON DELETE CASCADE NOT VALID
    1 7.6 ms 7.6 ms 7.6 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202105 ADD CONSTRAINT fk_rails_bb3355782d FOREIGN KEY (web_hook_id) REFERENCES web_hooks (id) ON DELETE CASCADE NOT VALID
    1 5.2 ms 5.2 ms 5.2 ms 0
    SELECT "postgres_partitioned_tables".*
      FROM "postgres_partitioned_tables"
      WHERE (identifier = concat(current_schema(), $1, $2))
      LIMIT $3 /*application:test*/
    1 4.4 ms 4.4 ms 4.4 ms 1
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202007 VALIDATE CONSTRAINT fk_rails_bb3355782d
    1 4.0 ms 4.0 ms 4.0 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202102 ADD CONSTRAINT fk_rails_bb3355782d FOREIGN KEY (web_hook_id) REFERENCES web_hooks (id) ON DELETE CASCADE NOT VALID
    1 2.5 ms 2.5 ms 2.5 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202103 ADD CONSTRAINT fk_rails_bb3355782d FOREIGN KEY (web_hook_id) REFERENCES web_hooks (id) ON DELETE CASCADE NOT VALID
    1 1.9 ms 1.9 ms 1.9 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202107 VALIDATE CONSTRAINT fk_rails_bb3355782d
    1 1.6 ms 1.6 ms 1.6 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202008 ADD CONSTRAINT fk_rails_bb3355782d FOREIGN KEY (web_hook_id) REFERENCES web_hooks (id) ON DELETE CASCADE NOT VALID
    1 1.6 ms 1.6 ms 1.6 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202105 VALIDATE CONSTRAINT fk_rails_bb3355782d
    1 1.4 ms 1.4 ms 1.4 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202106 VALIDATE CONSTRAINT fk_rails_bb3355782d
    1 1.3 ms 1.3 ms 1.3 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202108 VALIDATE CONSTRAINT fk_rails_bb3355782d
    1 1.3 ms 1.3 ms 1.3 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202101 ADD CONSTRAINT fk_rails_bb3355782d FOREIGN KEY (web_hook_id) REFERENCES web_hooks (id) ON DELETE CASCADE NOT VALID
    1 1.3 ms 1.3 ms 1.3 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202110 VALIDATE CONSTRAINT fk_rails_bb3355782d
    1 1.2 ms 1.2 ms 1.2 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202104 ADD CONSTRAINT fk_rails_bb3355782d FOREIGN KEY (web_hook_id) REFERENCES web_hooks (id) ON DELETE CASCADE NOT VALID
    1 1.2 ms 1.2 ms 1.2 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202110 ADD CONSTRAINT fk_rails_bb3355782d FOREIGN KEY (web_hook_id) REFERENCES web_hooks (id) ON DELETE CASCADE NOT VALID
    1 1.1 ms 1.1 ms 1.1 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202012 ADD CONSTRAINT fk_rails_bb3355782d FOREIGN KEY (web_hook_id) REFERENCES web_hooks (id) ON DELETE CASCADE NOT VALID
    1 1.0 ms 1.0 ms 1.0 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202009 ADD CONSTRAINT fk_rails_bb3355782d FOREIGN KEY (web_hook_id) REFERENCES web_hooks (id) ON DELETE CASCADE NOT VALID
    1 0.9 ms 0.9 ms 0.9 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202011 ADD CONSTRAINT fk_rails_bb3355782d FOREIGN KEY (web_hook_id) REFERENCES web_hooks (id) ON DELETE CASCADE NOT VALID
    1 0.9 ms 0.9 ms 0.9 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202109 ADD CONSTRAINT fk_rails_bb3355782d FOREIGN KEY (web_hook_id) REFERENCES web_hooks (id) ON DELETE CASCADE NOT VALID
    1 0.9 ms 0.9 ms 0.9 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202107 ADD CONSTRAINT fk_rails_bb3355782d FOREIGN KEY (web_hook_id) REFERENCES web_hooks (id) ON DELETE CASCADE NOT VALID
    1 0.9 ms 0.9 ms 0.9 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202007 ADD CONSTRAINT fk_rails_bb3355782d FOREIGN KEY (web_hook_id) REFERENCES web_hooks (id) ON DELETE CASCADE NOT VALID
    1 0.8 ms 0.8 ms 0.8 ms 0
    ALTER TABLE gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202010 ADD CONSTRAINT fk_rails_bb3355782d FOREIGN KEY (web_hook_id) REFERENCES web_hooks (id) ON DELETE CASCADE NOT VALID
    1 0.8 ms 0.8 ms 0.8 ms 0

    Artifacts


    Brought to you by gitlab-org/database-team/gitlab-com-database-testing. Contributions welcome! Epic

    • Author Contributor
      Resolved by Andreas Brandl

      @abrandl the good news is that by both checking the migration output and the recorded queries above, it's clear that adding the final foreign key to the partitioned table does not result to revalidating all the partitions :sparkler:

      So our approach is solid and works as expected.

      With respect to the time it takes to add the FKs to all the partitions, it's a lot but it should not affect the database (and hopefully it will be much faster on production but let's discuss without taking that into account).

      We could get down the rabbit hole of investigating solutions that would allow us to add the indexes and the FKs one partition at a time, split between deployments, but as partitions are dynamic for each instance that could be tricky and I am not sure if it is worth it.

  • Andreas Brandl added 396 commits

    added 396 commits

    • ef3d9b6e...2a5e720b - 395 commits from branch master
    • 326202f3 - Add FK to partitioned web_hook_logs

    Compare with previous version

  • Andreas Brandl approved this merge request

    approved this merge request

  • added databaseapproved label and removed databaseactive label

  • Yannis Roussos added 156 commits

    added 156 commits

    Compare with previous version

  • mentioned in epic &4785 (closed)

  • Yannis Roussos added 99 commits

    added 99 commits

    Compare with previous version

  • Author Contributor

    @abrandl the migrations for adding the indexes have reached production:

    gitlabhq_production=> \d web_hook_logs_part_0c5294f417
    ... ... ...
    Partition key: RANGE (created_at)
    Indexes:
        "web_hook_logs_part_0c5294f417_pkey" PRIMARY KEY, btree (id, created_at)
        "index_web_hook_logs_part_on_created_at_and_web_hook_id" btree (created_at, web_hook_id)
        "index_web_hook_logs_part_on_web_hook_id" btree (web_hook_id)
    Number of partitions: 17 (Use \d+ to list them.)
    
    gitlabhq_production=> \d gitlab_partitions_dynamic.web_hook_logs_part_0c5294f417_202104
    ... ... ...
    Partition of: web_hook_logs_part_0c5294f417 FOR VALUES FROM ('2021-04-01 00:00:00') TO ('2021-05-01 00:00:00')
    Indexes:
        "web_hook_logs_part_0c5294f417_202104_pkey" PRIMARY KEY, btree (id, created_at)
        "index_5dc0dc60d8" btree (created_at, web_hook_id)
        "index_d6756f0be7" btree (web_hook_id)

    I have made a small fix to cover the deprecation of sending hashes as params, but other than that everything is exactly the same as the version you already reviewed (I have of course tested that this change works as expected):

    --- a/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers.rb
    +++ b/lib/gitlab/database/partitioning_migration_helpers/foreign_key_helpers.rb
    @@ -59,11 +59,11 @@ def add_concurrent_partitioned_foreign_key(source, target, column:, on_delete: :
               partitioned_table = find_partitioned_table(source)
    
               partitioned_table.postgres_partitions.order(:name).each do |partition|
    -            add_concurrent_foreign_key(partition.identifier, target, partition_options)
    +            add_concurrent_foreign_key(partition.identifier, target, **partition_options)
               end
    
               with_lock_retries do
    -            add_foreign_key(source, target, partition_options)
    +            add_foreign_key(source, target, **partition_options)
               end
             end

    Do you mind reviewing the one last time and setting to MWPS if everything looks good to you?

  • Andreas Brandl resolved all threads

    resolved all threads

  • Andreas Brandl enabled an automatic merge when the pipeline for 5aaee635 succeeds

    enabled an automatic merge when the pipeline for 5aaee635 succeeds

  • Yannis Roussos aborted the automatic merge because source branch was updated

    aborted the automatic merge because source branch was updated

  • Yannis Roussos added 52 commits

    added 52 commits

    Compare with previous version

  • Yannis Roussos enabled an automatic merge when the pipeline for 4ac76e76 succeeds

    enabled an automatic merge when the pipeline for 4ac76e76 succeeds

  • Yannis Roussos mentioned in commit 887e7b54

    mentioned in commit 887e7b54

  • mentioned in issue #323676 (closed)

  • added workflowstaging label and removed workflowin review label

  • added workflowcanary label and removed workflowstaging label

  • added workflowproduction label and removed workflowcanary label

  • added devopsdata stores label and removed devopssystems label

Please register or sign in to reply
Loading