Add name to ci_builds_gitlab_monitor_metrics index
What does this MR do and why?
Preparation for filtering jobs by name !106458 (closed)
- Adds
name
column to index async - Removes
old
index async
Screenshots or screen recordings
Query testing results
Query
plan SELECT "ci_builds".* FROM "ci_builds" WHERE "ci_builds"."type" = 'Ci::Build' AND "ci_builds"."project_id" = 278964 AND ("ci_builds"."status" NOT IN ('created')) AND "ci_builds"."name" = 'detect-tests' AND ("ci_builds"."status" IN ('success')) ORDER BY "ci_builds"."id" DESC
Plan
Sort (cost=61096088.46..61096109.17 rows=8286 width=1250)
Sort Key: id DESC
-> Index Scan using "<13343>btree_ci_builds_status_created_at_project_id_name" on ci_builds (cost=0.08..61095549.19 rows=8286 width=1250)
Index Cond: (((status)::text = 'success'::text) AND (project_id = 278964) AND ((name)::text = 'detect-tests'::text))
Console using hypo
On instance https://console.postgres.ai/gitlab/joe-instances/114
How to set up and validate locally
Numbered steps to set up and validate the change are strongly suggested.
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Merge request reports
Activity
added backend database devopsverify grouppipeline execution labels
assigned to @pburdette
added sectionops label
mentioned in merge request !106458 (closed)
- A deleted user
added databasereview pending featureaddition typefeature labels
4 Warnings ⚠ New migrations added but db/structure.sql wasn't updated Usually, when adding new migrations, db/structure.sql should be
updated too (unless the migration isn't changing the DB schema
and isn't the most recent one).⚠ 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:
- The Handbook page on merge request types.
- The definition of done documentation.
⚠ 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 maintenancepipelines, maintenancerefactor, maintenanceworkflow, documentation, QA labels.⚠ This merge request does not refer to an existing milestone. 1 Message 📖 This merge request adds or changes files that require a review from the Database team. This merge request requires a database review. To make sure these changes are reviewed, take the following steps:
-
Ensure the merge request has database and databasereview pending labels. If the merge request modifies database files, Danger will do this for you.
-
Prepare your MR for database review according to the docs.
-
Assign and mention the database reviewer suggested by Reviewer Roulette.
-
Kick off the
db:gitlabcom-database-testing
manual job. This job can also be used before requesting review to test your migrations against production data.
The following files require a review from the Database team:
db/post_migrate/20221215150803_add_ci_builds_gitlab_monitor_metrics_v1_index.rb
db/post_migrate/20221215150847_remove_ci_builds_gitlab_monitor_metrics_index.rb
db/schema_migrations/20221215150803
db/schema_migrations/20221215150847
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 Nikola Milojevic (
@nmilojevic1
) (UTC+1, 6 hours ahead of@pburdette
)Alex Ives (
@alexives
) (UTC-6, 1 hour behind@pburdette
)~"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
🚫 Dangerrequested review from @praba.m7n
@praba.m7n as discussed in !106458 (closed) I've abstracted the DB changes into a standalone MR. Would you mind having a look at this one since you have context?
Thanks for making this @pburdette, code looks good to me
👍🏽 There is a failing job (db:check-migrations), but I think it's not related to the changes of this MR and since it has happened before too I have posted in #database channel to see if I can get more context around it (will update after I receive any clarification on this).
I have kicked off the gitlab-com-database-testing pipeline stage, will move it to the
maintainer-review
once it gets done.@pburdette - in addition to the
hypo
command, would you run anexec
and give us an estimation of how long the actual index creation will take?@alexives sure thing
👍 I'll get back to you once I have the results@alexives okay the results came back
1035.313 min
https://console.postgres.ai/gitlab/gitlab-production-ci/sessions/14034/commands/49010Thanks so much @pburdette!
added databasereviewed label and removed databasereview pending label
Database migrations (on the main database)
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 20221215150803 - AddCiBuildsGitlabMonitorMetricsV1Index Post deploy 2.6 s ✅ +8.00 KiB 20221215150847 - RemoveCiBuildsGitlabMonitorMetricsIndex Post deploy 2.4 s ✅ +0.00 B Runtime Histogram for all migrations
Query Runtime Count 0 seconds - 0.01 seconds 0 0.01 seconds - 0.1 seconds 2 0.1 seconds - 1 second 0 1 second - 5 seconds 0 5 seconds - 15 seconds 0 15 seconds - 5 minutes 0 5 minutes + 0 Migration: 20221215150803 - AddCiBuildsGitlabMonitorMetricsV1Index
- Type: Post deploy
- Duration: 2.6 s
- Database size change: +8.00 KiB
Query Calls Total Time Max Time Mean Time Rows INSERT INTO "postgres_async_indexes" ("created_at", "updated_at", "name", "definition", "table_name") VALUES ($1, $2, $3, $4, $5) RETURNING "id" /*application:test,db_config_name:main,line:/lib/gitlab/database/async_indexes/migration_helpers.rb:64:in `prepare_async_index'*/
1 2.2 ms 2.2 ms 2.2 ms 1 Histogram for AddCiBuildsGitlabMonitorMetricsV1Index
Query Runtime Count 0 seconds - 0.01 seconds 0 0.01 seconds - 0.1 seconds 1 0.1 seconds - 1 second 0 1 second - 5 seconds 0 5 seconds - 15 seconds 0 15 seconds - 5 minutes 0 5 minutes + 0 Migration: 20221215150847 - RemoveCiBuildsGitlabMonitorMetricsIndex
- Type: Post deploy
- Duration: 2.4 s
- Database size change: +0.00 B
Query Calls Total Time Max Time Mean Time Rows INSERT INTO "postgres_async_indexes" ("created_at", "updated_at", "name", "definition", "table_name") VALUES ($1, $2, $3, $4, $5) RETURNING "id" /*application:test,db_config_name:main,line:/lib/gitlab/database/async_indexes/migration_helpers.rb:98:in `prepare_async_index_removal'*/
1 0.1 ms 0.1 ms 0.1 ms 1 Histogram for RemoveCiBuildsGitlabMonitorMetricsIndex
Query Runtime Count 0 seconds - 0.01 seconds 0 0.01 seconds - 0.1 seconds 1 0.1 seconds - 1 second 0 1 second - 5 seconds 0 5 seconds - 15 seconds 0 15 seconds - 5 minutes 0 5 minutes + 0
Background migrations
Other migrations pending on GitLab.com
Migration Type Total runtime Result DB size change 20221124113925 - AddPipelineHierarchySizeToPlanLimits Regular 1.7 s ✅ +0.00 B 20221130170433 - CreateDastPreScanVerificationStep Regular 1.9 s ✅ +40.00 KiB 20221208122921 - RemoveConstraintsFromCiResourcesForPartitionId Regular 1.3 s ✅ +0.00 B 20221213064717 - ChangeDefaultPartitionIdOnCiResources Post deploy 1.6 s ✅ +0.00 B Clone Details
Clone ID Clone Created At Clone Data Timestamp Expected Removal Time database-testing-1608742-8773353-main
2022-12-15T17:58:55Z 2022-12-15T16:10:27Z 2022-12-16 06:02:40 +0000 database-testing-1608742-8773353-ci
2022-12-15T17:58:55Z 2022-12-15T16:45:29Z 2022-12-16 06:02:40 +0000 Artifacts
Brought to you by gitlab-org/database-team/gitlab-com-database-testing. Epic
Database migrations (on the ci database)
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 20221215150803 - AddCiBuildsGitlabMonitorMetricsV1Index Post deploy 3.3 s ✅ +8.00 KiB 20221215150847 - RemoveCiBuildsGitlabMonitorMetricsIndex Post deploy 3.0 s ✅ +0.00 B Runtime Histogram for all migrations
Query Runtime Count 0 seconds - 0.01 seconds 0 0.01 seconds - 0.1 seconds 2 0.1 seconds - 1 second 0 1 second - 5 seconds 0 5 seconds - 15 seconds 0 15 seconds - 5 minutes 0 5 minutes + 0 Migration: 20221215150803 - AddCiBuildsGitlabMonitorMetricsV1Index
- Type: Post deploy
- Duration: 3.3 s
- Database size change: +8.00 KiB
Query Calls Total Time Max Time Mean Time Rows INSERT INTO "postgres_async_indexes" ("created_at", "updated_at", "name", "definition", "table_name") VALUES ($1, $2, $3, $4, $5) RETURNING "id" /*application:test,db_config_name:ci,line:/lib/gitlab/database/async_indexes/migration_helpers.rb:64:in `prepare_async_index'*/
1 17.8 ms 17.8 ms 17.8 ms 1 Histogram for AddCiBuildsGitlabMonitorMetricsV1Index
Query Runtime Count 0 seconds - 0.01 seconds 0 0.01 seconds - 0.1 seconds 1 0.1 seconds - 1 second 0 1 second - 5 seconds 0 5 seconds - 15 seconds 0 15 seconds - 5 minutes 0 5 minutes + 0 Migration: 20221215150847 - RemoveCiBuildsGitlabMonitorMetricsIndex
- Type: Post deploy
- Duration: 3.0 s
- Database size change: +0.00 B
Query Calls Total Time Max Time Mean Time Rows INSERT INTO "postgres_async_indexes" ("created_at", "updated_at", "name", "definition", "table_name") VALUES ($1, $2, $3, $4, $5) RETURNING "id" /*application:test,db_config_name:ci,line:/lib/gitlab/database/async_indexes/migration_helpers.rb:98:in `prepare_async_index_removal'*/
1 0.1 ms 0.1 ms 0.1 ms 1 Histogram for RemoveCiBuildsGitlabMonitorMetricsIndex
Query Runtime Count 0 seconds - 0.01 seconds 0 0.01 seconds - 0.1 seconds 1 0.1 seconds - 1 second 0 1 second - 5 seconds 0 5 seconds - 15 seconds 0 15 seconds - 5 minutes 0 5 minutes + 0
Background migrations
Other migrations pending on GitLab.com
Migration Type Total runtime Result DB size change 20221213064717 - ChangeDefaultPartitionIdOnCiResources Post deploy 2.5 s ✅ +0.00 B Clone Details
Clone ID Clone Created At Clone Data Timestamp Expected Removal Time database-testing-1608742-8773353-main
2022-12-15T17:58:55Z 2022-12-15T16:10:27Z 2022-12-16 06:02:40 +0000 database-testing-1608742-8773353-ci
2022-12-15T17:58:55Z 2022-12-15T16:45:29Z 2022-12-16 06:02:40 +0000 Artifacts
Brought to you by gitlab-org/database-team/gitlab-com-database-testing. Epic
- A deleted user
added database-testing-automation label
👋 @praba.m7n
, thanks for approving this merge request.This is the first time the merge request is approved. To ensure full test coverage, a new pipeline will be started shortly.
For more info, please refer to the following links:
added pipeline:mr-approved label
@alexives - Can you please do a db-maintainer review on this?
@praba.m7n Sure thing! Don't forget to assign it to me for review!
removed review request for @praba.m7n
requested review from @alexives
removed review request for @alexives
requested review from @alexives
marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed
removed review request for @alexives
requested review from @alexives
@pburdette thanks for your patence here! I'd like to get one more opinion on this index change before we proceed.
@mbobin could I get your advice on this? After chatting with you in slack briefly it sounds like we'd rather still avoid changing indexes on ci_builds where we can and this index took ~16 hours to build on database lab.
@alexives the current index takes 130GB and if we add
name
to it it might increase by another 100GB and this is a problem:- this index is already too large, our guidelines state that we should not have tables over 100GB in size.
- it's not clear if the async index will be created successfully because we've tried to change an index for partitioning and it failed. Cleaning this up is not easy, we've tried this weekend to reschedule it, but for some unknown reason it failed again.
- each weekend we try to maintain the largest indexes on
ci_builds
but it can create incidents if the task stretches into Monday to complete. We've had one this morning: gitlab-com/gl-infra/production#8174 (closed) and a few more in the recent past... - I'm not convinced that the index would cover every filter scenario and if it's applicable to name filtering since it has
created_at
in the middle and we don't use that for filtering.🤔
Also we'll try to recreate the index for partitioning this weekend again and probably we don't want anything else to get in the way since it's a short window in which it can be done.
😅 /cc @morefice
I'm not convinced that the index would cover every filter scenario and if it's applicable to name filtering since it has
created_at
in the middle and we don't use that for filtering.@mbobin the query used to filter would use this index, I'm confused on this statement
🤔 Sounds like we should wait until moving forward with this feature !106458 (closed)
How long we should wait though, I'm not sure. Could you point me to blocking issues that need to be solved first?
the query used to filter would use this index, I'm confused on this statement
@pburdette I mean that it is possible to use it, but it will not be used efficiently. The columns in an index are ordered, so for an indexed to be used efficiently(reading the minimum amount of data) the query must match the columns, it can not omit a column from the beginning or middle of the index. For example an index with
:status, :created_at, :project_id, :name
would allow filtering bystatus
,status and created_at
,:status, :created_at, and :project_id
, and finally:status, :created_at, :project_id, and :name
. If we don't supply thecreated_at
value in the query, the planner might chose to use the index, but it will be a full index scan that will use only:status
to effectively speed up the query. A recent discussion about this: https://gitlab.slack.com/archives/CNZ8E900G/p1673288556636859How long we should wait though, I'm not sure. Could you point me to blocking issues that need to be solved first?
I think it will be 2-3 milestones(optimistic) out because we need to partition the
ci_builds
table, create a new partition and start using that partition. I don't think we have issues yet for the last two steps.Edited by Marius Bobin@mbobin should I close this MR? Or are we getting close to having the partitioning done?
requested review from @mbobin
removed review request for @alexives
removed review request for @mbobin
mentioned in issue #22626 (closed)
mentioned in issue gitlab-org/quality/triage-reports#11318 (closed)
mentioned in issue gitlab-org/quality/triage-reports#11521 (closed)
mentioned in issue gitlab-org/quality/triage-reports#11941 (closed)
mentioned in issue gitlab-org/quality/triage-reports#13181 (closed)
mentioned in issue #416374
mentioned in issue #22027
mentioned in issue #423904 (closed)
Closing now in favor of #423904 (closed)
Hello @pburdette
👋 The database team is looking for ways to improve the database review process and we would love your help!
If you'd be open to someone on the database team reaching out to you for a chat, or if you'd like to leave some feedback asynchronously, just post a reply to this comment mentioning:
@gitlab-org/database-team
And someone will be by shortly!
Thanks for your help!
❤ This message was generated automatically. You're welcome to improve it.
reset approvals from @praba.m7n by pushing to the branch