Skip to content
Snippets Groups Projects

Finalize conversion to bigint for events

Merged Krasimir Angelov requested to merge 288005-finalize-events-bigint-conversion into master

What does this MR do?

This MR finalize the conversion to bigint for events table.

At a high level, the operation takes the following steps:

  1. Ensure the migration of id to id_convert_to_bigint is completed.
  2. Copy indexes and FKs
  3. Swap columns

Cleanup (removing the old int columns and the triggers) will be done in later MR. This leaves us with a trigger that copy values from bigint column to integer column, but this works fine, as long we do not hit the limit for integer.

Related to #288005 (closed).

:exclamation: Before merging

  • Make sure !68426 (merged) is deployed to production and indexes are created.

Database migrations

:stopwatch: Timing

From DB Lab:

  1. ALTER TABLE push_event_payloads VALIDATE CONSTRAINT fk_36c74129da_tmp

    Duration: 22.554 min - https://console.postgres.ai/gitlab/gitlab-production-tunnel-pg12/sessions/4907/commands/17161

Indexes were pre-created with !68426 (comment 657250665), and now the only time consuming statement is the FK validation. This is also confirmed from the migration testing pipeline - !64779 (comment 613785586).

:notepad_spiral: Output

:arrow_up: Up
$ bundle exec rails db:migrate:up VERSION=20210622045705
== 20210622045705 FinalizeEventsBigintConversion: migrating ===================
-- transaction_open?()
   -> 0.0000s
-- index_exists?("events", :id_convert_to_bigint, {:unique=>true, :name=>"index_events_on_id_convert_to_bigint", :algorithm=>:concurrently})
   -> 0.0046s
-- execute("SET statement_timeout TO 0")
   -> 0.0005s
-- add_index("events", :id_convert_to_bigint, {:unique=>true, :name=>"index_events_on_id_convert_to_bigint", :algorithm=>:concurrently})
   -> 0.0100s
-- execute("RESET statement_timeout")
   -> 0.0006s
-- transaction_open?()
   -> 0.0000s
-- index_exists?("events", [:project_id, :id_convert_to_bigint], {:name=>"index_events_on_project_id_and_id_convert_to_bigint", :algorithm=>:concurrently})
   -> 0.0042s
-- add_index("events", [:project_id, :id_convert_to_bigint], {:name=>"index_events_on_project_id_and_id_convert_to_bigint", :algorithm=>:concurrently})
   -> 0.0027s
-- transaction_open?()
   -> 0.0000s
-- index_exists?("events", [:project_id, :id_convert_to_bigint], {:order=>{:id_convert_to_bigint=>:desc}, :where=>"action = 7", :name=>"index_events_on_project_id_and_id_bigint_desc_on_merged_action", :algorithm=>:concurrently})
   -> 0.0049s
-- add_index("events", [:project_id, :id_convert_to_bigint], {:order=>{:id_convert_to_bigint=>:desc}, :where=>"action = 7", :name=>"index_events_on_project_id_and_id_bigint_desc_on_merged_action", :algorithm=>:concurrently})
   -> 0.0035s
-- transaction_open?()
   -> 0.0000s
-- foreign_keys(:push_event_payloads)
   -> 0.0043s
-- execute("LOCK TABLE events, push_event_payloads IN SHARE ROW EXCLUSIVE MODE")
   -> 0.0007s
-- execute("ALTER TABLE push_event_payloads\nADD CONSTRAINT fk_36c74129da_tmp\nFOREIGN KEY (event_id)\nREFERENCES events (id_convert_to_bigint)\nON DELETE CASCADE\nNOT VALID;\n")
   -> 0.0085s
-- execute("ALTER TABLE push_event_payloads VALIDATE CONSTRAINT fk_36c74129da_tmp;")
   -> 0.0082s
-- execute("LOCK TABLE events, push_event_payloads IN ACCESS EXCLUSIVE MODE")
   -> 0.0006s
-- quote_table_name("events")
   -> 0.0000s
-- quote_column_name(:id)
   -> 0.0000s
-- quote_column_name("id_tmp")
   -> 0.0000s
-- execute("ALTER TABLE \"events\" RENAME COLUMN \"id\" TO \"id_tmp\"")
   -> 0.0006s
-- quote_table_name("events")
   -> 0.0000s
-- quote_column_name(:id_convert_to_bigint)
   -> 0.0000s
-- quote_column_name(:id)
   -> 0.0000s
-- execute("ALTER TABLE \"events\" RENAME COLUMN \"id_convert_to_bigint\" TO \"id\"")
   -> 0.0006s
-- quote_table_name("events")
   -> 0.0000s
-- quote_column_name("id_tmp")
   -> 0.0000s
-- quote_column_name(:id_convert_to_bigint)
   -> 0.0000s
-- execute("ALTER TABLE \"events\" RENAME COLUMN \"id_tmp\" TO \"id_convert_to_bigint\"")
   -> 0.0005s
-- quote_table_name("trigger_69523443cc10")
   -> 0.0000s
-- execute("ALTER FUNCTION \"trigger_69523443cc10\" RESET ALL")
   -> 0.0006s
-- execute("ALTER SEQUENCE events_id_seq OWNED BY events.id")
   -> 0.0010s
-- change_column_default("events", :id, #<Proc:0x00007feb36f15378 /Users/krasio/projects/gitlab/ee/gitlab/db/post_migrate/20210622045705_finalize_events_bigint_conversion.rb:68 (lambda)>)
   -> 0.0048s
-- change_column_default("events", :id_convert_to_bigint, 0)
   -> 0.0019s
-- execute("ALTER TABLE events DROP CONSTRAINT events_pkey CASCADE")
   -> 0.0015s
-- rename_index("events", "index_events_on_id_convert_to_bigint", "events_pkey")
   -> 0.0007s
-- execute("ALTER TABLE events ADD CONSTRAINT events_pkey PRIMARY KEY USING INDEX events_pkey")
   -> 0.0013s
-- execute("DROP INDEX index_events_on_project_id_and_id")
   -> 0.0007s
-- rename_index("events", "index_events_on_project_id_and_id_convert_to_bigint", "index_events_on_project_id_and_id")
   -> 0.0012s
-- execute("DROP INDEX index_events_on_project_id_and_id_desc_on_merged_action")
   -> 0.0007s
-- rename_index("events", "index_events_on_project_id_and_id_bigint_desc_on_merged_action", "index_events_on_project_id_and_id_desc_on_merged_action")
   -> 0.0007s
-- quote_table_name(:push_event_payloads)
   -> 0.0000s
-- quote_column_name("fk_36c74129da_tmp")
   -> 0.0000s
-- quote_column_name("fk_36c74129da")
   -> 0.0000s
-- execute("ALTER TABLE \"push_event_payloads\"\nRENAME CONSTRAINT \"fk_36c74129da_tmp\" TO \"fk_36c74129da\"\n")
   -> 0.0006s
== 20210622045705 FinalizeEventsBigintConversion: migrated (0.1288s) ==========
:arrow_down: Down
$ bundle exec rails db:migrate:down VERSION=20210622045705
== 20210622045705 FinalizeEventsBigintConversion: reverting ===================
-- transaction_open?()
   -> 0.0000s
-- index_exists?("events", :id_convert_to_bigint, {:unique=>true, :name=>"index_events_on_id_convert_to_bigint", :algorithm=>:concurrently})
   -> 0.0048s
-- execute("SET statement_timeout TO 0")
   -> 0.0005s
-- add_index("events", :id_convert_to_bigint, {:unique=>true, :name=>"index_events_on_id_convert_to_bigint", :algorithm=>:concurrently})
   -> 0.0043s
-- execute("RESET statement_timeout")
   -> 0.0005s
-- transaction_open?()
   -> 0.0000s
-- index_exists?("events", [:project_id, :id_convert_to_bigint], {:name=>"index_events_on_project_id_and_id_convert_to_bigint", :algorithm=>:concurrently})
   -> 0.0033s
-- add_index("events", [:project_id, :id_convert_to_bigint], {:name=>"index_events_on_project_id_and_id_convert_to_bigint", :algorithm=>:concurrently})
   -> 0.0021s
-- transaction_open?()
   -> 0.0000s
-- index_exists?("events", [:project_id, :id_convert_to_bigint], {:order=>{:id_convert_to_bigint=>:desc}, :where=>"action = 7", :name=>"index_events_on_project_id_and_id_bigint_desc_on_merged_action", :algorithm=>:concurrently})
   -> 0.0038s
-- add_index("events", [:project_id, :id_convert_to_bigint], {:order=>{:id_convert_to_bigint=>:desc}, :where=>"action = 7", :name=>"index_events_on_project_id_and_id_bigint_desc_on_merged_action", :algorithm=>:concurrently})
   -> 0.0022s
-- transaction_open?()
   -> 0.0000s
-- foreign_keys(:push_event_payloads)
   -> 0.0027s
-- execute("LOCK TABLE events, push_event_payloads IN SHARE ROW EXCLUSIVE MODE")
   -> 0.0006s
-- execute("ALTER TABLE push_event_payloads\nADD CONSTRAINT fk_36c74129da_tmp\nFOREIGN KEY (event_id)\nREFERENCES events (id_convert_to_bigint)\nON DELETE CASCADE\nNOT VALID;\n")
   -> 0.0013s
-- execute("ALTER TABLE push_event_payloads VALIDATE CONSTRAINT fk_36c74129da_tmp;")
   -> 0.0027s
-- execute("LOCK TABLE events, push_event_payloads IN ACCESS EXCLUSIVE MODE")
   -> 0.0005s
-- quote_table_name("events")
   -> 0.0000s
-- quote_column_name(:id)
   -> 0.0000s
-- quote_column_name("id_tmp")
   -> 0.0000s
-- execute("ALTER TABLE \"events\" RENAME COLUMN \"id\" TO \"id_tmp\"")
   -> 0.0005s
-- quote_table_name("events")
   -> 0.0000s
-- quote_column_name(:id_convert_to_bigint)
   -> 0.0000s
-- quote_column_name(:id)
   -> 0.0000s
-- execute("ALTER TABLE \"events\" RENAME COLUMN \"id_convert_to_bigint\" TO \"id\"")
   -> 0.0005s
-- quote_table_name("events")
   -> 0.0000s
-- quote_column_name("id_tmp")
   -> 0.0000s
-- quote_column_name(:id_convert_to_bigint)
   -> 0.0000s
-- execute("ALTER TABLE \"events\" RENAME COLUMN \"id_tmp\" TO \"id_convert_to_bigint\"")
   -> 0.0006s
-- quote_table_name("trigger_69523443cc10")
   -> 0.0000s
-- execute("ALTER FUNCTION \"trigger_69523443cc10\" RESET ALL")
   -> 0.0007s
-- execute("ALTER SEQUENCE events_id_seq OWNED BY events.id")
   -> 0.0006s
-- change_column_default("events", :id, #<Proc:0x00007fcaeedc3e60 /Users/krasio/projects/gitlab/ee/gitlab/db/post_migrate/20210622045705_finalize_events_bigint_conversion.rb:68 (lambda)>)
   -> 0.0030s
-- change_column_default("events", :id_convert_to_bigint, 0)
   -> 0.0018s
-- execute("ALTER TABLE events DROP CONSTRAINT events_pkey CASCADE")
   -> 0.0009s
-- rename_index("events", "index_events_on_id_convert_to_bigint", "events_pkey")
   -> 0.0006s
-- execute("ALTER TABLE events ADD CONSTRAINT events_pkey PRIMARY KEY USING INDEX events_pkey")
   -> 0.0006s
-- execute("DROP INDEX index_events_on_project_id_and_id")
   -> 0.0006s
-- rename_index("events", "index_events_on_project_id_and_id_convert_to_bigint", "index_events_on_project_id_and_id")
   -> 0.0006s
-- execute("DROP INDEX index_events_on_project_id_and_id_desc_on_merged_action")
   -> 0.0006s
-- rename_index("events", "index_events_on_project_id_and_id_bigint_desc_on_merged_action", "index_events_on_project_id_and_id_desc_on_merged_action")
   -> 0.0006s
-- quote_table_name(:push_event_payloads)
   -> 0.0000s
-- quote_column_name("fk_36c74129da_tmp")
   -> 0.0000s
-- quote_column_name("fk_36c74129da")
   -> 0.0000s
-- execute("ALTER TABLE \"push_event_payloads\"\nRENAME CONSTRAINT \"fk_36c74129da_tmp\" TO \"fk_36c74129da\"\n")
   -> 0.0006s
== 20210622045705 FinalizeEventsBigintConversion: reverted (0.0680s) ==========

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

Does this MR contain changes to processing or storing of credentials or tokens, authorization and authentication methods or other items described in the security review guidelines? If not, then delete this Security section.

  • 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

Related to #288005 (closed)

Edited by Krasimir Angelov

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
  • Krasimir Angelov changed milestone to %14.1

    changed milestone to %14.1

  • assigned to @krasio

  • A deleted user added backend label

    added backend label

  • 1 Warning
    :warning: You've made some app changes, but didn't add any tests.
    That's OK as long as you're refactoring existing code,
    but please consider adding any of the tooling, ~"tooling::pipelines", ~"tooling::workflow", documentation, QA labels.

    Reviewer roulette

    Changes that require review have been detected!

    Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:

    Category Reviewer Maintainer
    database Alina Mihaila (@alinamihaila) (UTC+3, 9 hours behind @krasio) Tiger Watson (@tigerwnz) (UTC+10, 2 hours behind @krasio)
    ~migration No reviewer available No maintainer available

    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.

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

    Generated by :no_entry_sign: Danger

  • Krasimir Angelov changed the description

    changed the description

  • Krasimir Angelov changed target branch from master to 288005-finalize-push-events-payloads-bigint-conversion

    changed target branch from master to 288005-finalize-push-events-payloads-bigint-conversion

  • mentioned in epic &4785 (closed)

  • Krasimir Angelov added 5 commits

    added 5 commits

    • 33b7e614...73b764b8 - 3 commits from branch 288005-finalize-push-events-payloads-bigint-conversion
    • e398644c - Improve add_concurrent_foreign_key to support custom target column
    • 0f5cc3fc - Finalize conversion to bigint for events

    Compare with previous version

  • Krasimir Angelov changed the description

    changed the description

  • Krasimir Angelov changed the description

    changed the description

  • mentioned in issue #288005 (closed)

  • Krasimir Angelov deleted the 288005-finalize-push-events-payloads-bigint-conversion branch. This merge request now targets the master branch

    deleted the 288005-finalize-push-events-payloads-bigint-conversion branch. This merge request now targets the master branch

  • Krasimir Angelov added 560 commits

    added 560 commits

    • 0f5cc3fc...ea525f3b - 558 commits from branch master
    • 31f6fd19 - Improve add_concurrent_foreign_key to support custom target column
    • 2f483c22 - Finalize conversion to bigint for events

    Compare with previous version

  • Krasimir Angelov changed the description

    changed the description

  • Krasimir Angelov changed the description

    changed the description

  • Database migrations

    3 Warnings
    :warning: 20210622045705 - FinalizeEventsBigintConversion had a query that exceeded timing guidelines.
    Run time should not exceed 100ms, but was it was 1693168.93ms. Please consider possible options to
    improve the query performance.
    ALTER TABLE push_event_payloads VALIDATE CONSTRAINT
    fk_36c74129da_tmp
    :warning: 20210622045705 - FinalizeEventsBigintConversion took 28.5min. Please add a comment that
    mentions Release Managers (@gitlab-org/release/managers) so they are informed.
    :warning: 20210622045705 - FinalizeEventsBigintConversion may need a background migration to comply
    with timing guidelines. It took 28.5min, but should not exceed 10.0min

    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).

    Migration Type Total runtime Result DB size change
    20210622045705 - FinalizeEventsBigintConversion Post deploy 1710.6 s :warning: -56.19 GiB
    Runtime Histogram for all migrations
    Query Runtime Count
    0 seconds - 0.01 seconds 0
    0.01 seconds - 0.1 seconds 100
    0.1 seconds - 1 second 1
    1 second - 5 minutes 1
    5 minutes + 1

    :warning: Migration: 20210622045705 - FinalizeEventsBigintConversion

    • Type: Post deploy
    • Duration: 1710.6 s
    • Database size change: -56.19 GiB
    Query Calls Total Time Max Time Mean Time Rows
    ALTER TABLE push_event_payloads VALIDATE CONSTRAINT fk_36c74129da_tmp
    1 1693168.9 ms 1693168.9 ms 1693168.9 ms 0
    ALTER TABLE push_event_payloads ADD CONSTRAINT fk_36c74129da_tmp FOREIGN KEY (event_id) REFERENCES events (id_convert_to_bigint) ON DELETE CASCADE NOT VALID
    1 38.7 ms 38.7 ms 38.7 ms 0
    ALTER TABLE "events" ALTER COLUMN "id" SET DEFAULT nextval('events_id_seq'::regclass) /*application:test*/
    1 31.0 ms 31.0 ms 31.0 ms 0
    ALTER TABLE events DROP CONSTRAINT events_pkey CASCADE /*application:test*/
    1 23.3 ms 23.3 ms 23.3 ms 0
    ALTER SEQUENCE events_id_seq OWNED BY events.id /*application:test*/
    1 17.4 ms 17.4 ms 17.4 ms 0
    ALTER TABLE events ADD CONSTRAINT events_pkey PRIMARY KEY USING INDEX events_pkey /*application:test*/
    1 16.2 ms 16.2 ms 16.2 ms 0
    ALTER TABLE "events" RENAME COLUMN "id_convert_to_bigint" TO "id" /*application:test*/
    1 14.6 ms 14.6 ms 14.6 ms 0
    ALTER TABLE "events" RENAME COLUMN "id" TO "id_tmp" /*application:test*/
    1 9.4 ms 9.4 ms 9.4 ms 0
    ALTER FUNCTION "trigger_69523443cc10" RESET ALL /*application:test*/
    1 4.6 ms 4.6 ms 4.6 ms 0
    DROP INDEX index_events_on_project_id_and_id /*application:test*/
    1 4.6 ms 4.6 ms 4.6 ms 0
    DROP INDEX index_events_on_project_id_and_id_desc_on_merged_action /*application:test*/
    1 4.5 ms 4.5 ms 4.5 ms 0
    ALTER INDEX "index_events_on_id_convert_to_bigint" RENAME TO "events_pkey" /*application:test*/
    1 4.2 ms 4.2 ms 4.2 ms 0
    ALTER INDEX "index_events_on_project_id_and_id_convert_to_bigint" RENAME TO "index_events_on_project_id_and_id" /*application:test*/
    1 3.5 ms 3.5 ms 3.5 ms 0
    SELECT "batched_background_migrations".*
    FROM "batched_background_migrations"
    WHERE "batched_background_migrations"."job_class_name" = $1 AND "batched_background_migrations"."table_name" = $2 AND "batched_background_migrations"."column_name" = $3 AND (job_arguments = $4)
    ORDER BY "batched_background_migrations"."id" ASC
    LIMIT $5 /*application:test*/
    1 3.2 ms 3.2 ms 3.2 ms 1
    ALTER TABLE "events" ALTER COLUMN "id_convert_to_bigint" SET DEFAULT 0 /*application:test*/
    1 3.1 ms 3.1 ms 3.1 ms 0
    ALTER TABLE "events" RENAME COLUMN "id_tmp" TO "id_convert_to_bigint" /*application:test*/
    1 2.7 ms 2.7 ms 2.7 ms 0
    LOCK TABLE events, push_event_payloads IN SHARE ROW EXCLUSIVE MODE /*application:test*/
    1 0.4 ms 0.4 ms 0.4 ms 0
    ALTER TABLE "push_event_payloads" RENAME CONSTRAINT "fk_36c74129da_tmp" TO "fk_36c74129da"/*application:test*/
    1 0.4 ms 0.4 ms 0.4 ms 0
    ALTER INDEX "index_events_on_project_id_and_id_bigint_desc_on_merged_action" RENAME TO "index_events_on_project_id_and_id_desc_on_merged_action" /*application:test*/
    1 0.3 ms 0.3 ms 0.3 ms 0
    LOCK TABLE events, push_event_payloads IN ACCESS EXCLUSIVE MODE /*application:test*/
    1 0.2 ms 0.2 ms 0.2 ms 0
    Histogram for FinalizeEventsBigintConversion
    Query Runtime Count
    0 seconds - 0.01 seconds 0
    0.01 seconds - 0.1 seconds 100
    0.1 seconds - 1 second 1
    1 second - 5 minutes 1
    5 minutes + 1

    Other migrations pending on GitLab.com
    Migration Type Total runtime Result DB size change
    20210706112800 - RemoveCloudLicenseEnabledFromApplicationSettings Post deploy 1.3 s :white_check_mark: +0.00 B

    Clone Details

    Clone ID Clone Created At Clone Data Timestamp Expected Removal Time
    database-testing-757600 2021-08-24 02:36:05 UTC 2021-08-24 00:01:03 UTC 2021-08-24 15:06:33 +0000

    Artifacts


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

  • added 1 commit

    • 21e1c578 - Finalize conversion to bigint for events

    Compare with previous version

  • Krasimir Angelov changed the description

    changed the description

  • Author Maintainer

    @engwan @abrandl Can you please do backend and database review?

  • Krasimir Angelov requested review from @engwan and @abrandl

    requested review from @engwan and @abrandl

  • added workflowin review label and removed workflowin dev label

  • Krasimir Angelov changed the description

    changed the description

  • Heinrich Lee Yu approved this merge request

    approved this merge request

  • Backend looks good to me :thumbsup:

  • Andreas Brandl resolved all threads

    resolved all threads

  • Andreas Brandl changed the description

    changed the description

  • Andreas Brandl approved this merge request

    approved this merge request

  • Created gitlab-com/gl-infra/production#5034 (closed) for creating indexes upfront

  • Andreas Brandl resolved all threads

    resolved all threads

  • Andreas Brandl resolved all threads

    resolved all threads

  • Andreas Brandl added 1 commit

    added 1 commit

    • 0c130139 - Apply 2 suggestion(s) to 1 file(s)

    Compare with previous version

  • Database looks good, too! :thumbsup: I only added a cosmetic suggestion.

    I'll get gitlab-com/gl-infra/production#5034 (closed) going and once indexes are there, we can merge here. :rocket:

    Thanks @krasio!

  • mentioned in issue #334823 (closed)

  • Heinrich Lee Yu removed review request for @engwan

    removed review request for @engwan

  • Krasimir Angelov changed the description

    changed the description

  • Andreas Brandl marked this merge request as ready

    marked this merge request as ready

  • Andreas Brandl marked the checklist item Execute upon gitlab-com/gl-infra/production#5034 (closed) as completed

    marked the checklist item Execute upon gitlab-com/gl-infra/production#5034 (closed) as completed

  • Andreas Brandl added 539 commits

    added 539 commits

    • 0c130139...a5c95756 - 536 commits from branch master
    • 12a07084 - Improve add_concurrent_foreign_key to support custom target column
    • 23e53a66 - Finalize conversion to bigint for events
    • afc9b1e5 - Apply 2 suggestion(s) to 1 file(s)

    Compare with previous version

  • Yannis Roussos marked this merge request as draft

    marked this merge request as draft

  • I switched the draft status to avoid this being accidentally merged and moving to %14.2 per #288005 (comment 616613886)

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading