Database Trainee Maintainer: Simon Tomlinson

Basic Setup

Thank you for becoming a database trainee! Please work on the list below to complete your setup.

• Change issue title to include your name: Database Trainee Maintainer: Your Name
• Review general code review guidelines
• Familiarize with database review process
• Familiarize with migration helpers and review usage in existing migrations
• Familiarize with best practices in database guides
• Watch Optimising Rails Database Queries: Episode 1
• Read Understanding EXPLAIN plans
• Get yourself added to @gl-database group and respond to @-mentions to the group (reach out to any maintainer on the group to get added). You will get TODOs on gitlab.com for group mentions.
• Make sure you have proper access to at least an archive replica in production and a read-only replica in staging and production
• Indicate in your team member entry your role as a database reviewer (example MR). Assign MR to your manager for merge.
• Once you are comfortable as a reviewer add trainee_maintainer database to your role in your team member entry. Trainee maintainers count as reviewers and are three times as likely to be picked as other reviewers
• Send one MR to improve the review documentation

Note that approving and accepting merge requests is restricted to Database Maintainers only. As a reviewer, pass the MR to a maintainer for approval.

You're all set! Watch out for TODOs on GitLab.com.

Working towards becoming a maintainer

There is no checklist here, only guidelines. Remember that there is no specific timeline on this.

It is up to you to ensure that you are getting enough MRs to review, and of varied types. After you've added yourself as a Database Reviewer, you should already be receiving regular reviews from Reviewer Roulette. You could also seek out more reviews from your team, or on #backend/#database Slack channels.

Your reviews should aim to cover maintainer responsibilities as well as reviewer responsibilities. Your approval means you think it is ready to merge.

After each MR is merged or closed, add a discussion to this issue using this template:

### (Merge request title): (Merge request URL)

During review:

- (List anything of note, or a quick summary. "I suggested/identified/noted...")

Post-review:

- (List anything of note, or a quick summary. "I missed..." or "Merged as-is")

(Maintainer who reviewed this merge request) Please add feedback, and compare
this review to the average maintainer review.

Note: Do not include reviews of security MRs because review feedback might reveal security issue details.

Tip: There are tools available to assist with this task.

When you're ready to make it official

When reviews have accumulated, and recent reviews consistently fulfill maintainer responsibilities, any maintainer can take the next step. The trainee should also feel free to discuss their progress with their manager or any maintainer at any time.

1. Create a merge request updating your team member entry using the template proposing yourself as a database maintainer, assigned to your manager.
2. Get yourself added to @gitlab-org/maintainers/database group (this group is recognized in CODEOWNERS). Reach out to any existing maintainer.
3. Keep reviewing, start merging

Where to go for questions?

Reach out to #database on Slack.

added Database trainee maintainer labels

assigned to @stomlinson

marked this issue as related to gitlab-org/database-team/team-tasks#184 (closed)

marked the checklist item Change issue title to include your name: Database Trainee Maintainer: Your Name as completed

marked the checklist item Get yourself added to @gl-database group and respond to @-mentions to the group (reach out to any maintainer on the group to get added). You will get TODOs on gitlab.com for group mentions. as completed

marked the checklist item Indicate in your team member entry your role as a database reviewer (example MR). Assign MR to your manager for merge. as completed

marked the checklist item Read Understanding EXPLAIN plans as completed

marked the checklist item Review general code review guidelines as completed

marked the checklist item Familiarize with database review process as completed

marked the checklist item Familiarize with migration helpers and review usage in existing migrations as completed

marked the checklist item Familiarize with best practices in database guides as completed

marked the checklist item Watch Optimising Rails Database Queries: Episode 1 as completed

mentioned in merge request !86795 (merged)

marked the checklist item Once you are comfortable as a reviewer add trainee_maintainer database to your role as completed

Add optional owned_only param to namespaces endpoint : gitlab-org/gitlab!66166 (merged)

During review:

• I noticed a suboptimal query plan due to an x OR y type query, and suggested that it be refactored to a UNION so that postgres would use an index.

• I noticed that the query plans provided used user and namespace ID 1, which I thought might provide a non-realistic plan, so I asked for a plan using the author's user, or a similar one.

  • The author pointed out that user 1 / namespace 1 had plenty of associated rows. In the future I'll double check the rows in the query plan before asking for a different choice of input data.

Post-review:

• Merged as-is.

@toon Please add feedback, and compare this review to the average maintainer review.

Generate Debian component files for udeb and source files : gitlab-org/gitlab!65188 (merged)

During review:

• I noticed two places where we were already joining to tables that were later preload()ed, but one was not part of the diff and I wasn't able to convince rails to eager load through an inner join when trying the other one, so I decided not to mention the optimization. It wasn't a performance critical place and the current code seemed easier to read.

Post-review:

• Merged as-is

@sabrams Please add feedback, and compare this review to the average maintainer review.

Nothing to add from the review standpoint. Thanks for sharing your thoughts on your process here. It never hurts to include comments like that in the MRs too if you think they may benefit others. If it turns into a concern, a followup can be opened, or at the least, you may educate the author or other reviewers on something they might not have considered.

Add vulnerabilities_allowed column : gitlab-org/gitlab!66673 (merged)

During review:

• I suggested a more meaningful name for the column being added

Post-review:

• Merged as-is

@sabrams Please add feedback, and compare this review to the average maintainer review.

Add overridden_uuid column to security_findings : gitlab-org/gitlab!66689 (merged)

During review:

• I had no comments

Post-review:

• Merged as-is

@sabrams Please add feedback, and compare this review to the average maintainer review.

Finalize conversion to bigint for ci_build_needs : gitlab-org/gitlab!65216 (merged)

During review:

• I had no comments

Post-review:

• We had deadlocking issues when deploying a different bigint migration after my review, so @pbair asked about locking the tables in parent-child order.

I began looking for opportunities to prevent deadlocks once we became aware of them.

@pbair Please add feedback, and compare this review to the average maintainer review.

Not much to comment here. I don't think anyone suspected the related migration might deadlock, so I wouldn't expect for you to have called it out ahead of time.

Add severity_levels column into approval_project_rules table : gitlab-org/gitlab!67062 (merged)

During review:

• I was concerned with denormalization by adding a text[] column to the approval_project_rules table, but tried to balance that with simplicity of implementation. I wasn't sure how to proceed, so I laid out ideas for either keeping a text[] column or splitting out a new table and asked the maintainer for advice.

Post-Review:

• @sabrams agreed that we should use a text[] column for now and could migrate away if we need to later.
• I missed that we should default to an empty array rather than null to make rails code simpler (so that it does not have to deal with two data types).

@sabrams Please add feedback, and compare this review to the average maintainer review.

Log message when async index creation completes : gitlab-org/gitlab!68092 (merged)

During review:

• I had no comments

Post-review:

• Merged as-is

@mayra-cabrera Please add feedback, and compare this review to the average maintainer review

Small change. Nothing to add.

Feature(integrate zentao): part of db migrations : !67938 (merged)

During review:

• I asked that an external ID be renamed to an _xid suffix following the pattern in https://docs.gitlab.com/ee/development/foreign_keys.html#naming-foreign-keys

Post-review:

• @mayra-cabrera suggested that auto-generated comments be removed from the migration, that create_table_with_constraints be switched to create_table so that we could use a change method, and that with_lock_retries be eliminated from the down method since the migration doesn't touch a high-traffic table.

I had noticed the comments, but thought that since they were auto-generated by the migration tool they were fine to leave in. I'll start requesting that people remove them in future reviews.

I didn't notice the with_lock_retries issue in the down migration, so I'll start looking for that as well.

I didn't notice that create_table_with_constraints could be swapped out for the simpler create_table - I thought that some part of the foreign key definition inline was using the _with_constraints version. I've read the create_table_with_constraints source now, so I understand that it's only for check constraints.

@mayra-cabrera Please add feedback, and compare this review to the average maintainer review.

Feature(integrate zentao): part of db migrations : !67938 (merged)

I think you're referencing another MR? :)

I had noticed the comments, but thought that since they were auto-generated by the migration tool they were fine to leave in. I'll start requesting that people remove them in future reviews.

Auto-generated comments are noisy, and after they've been read and addressed they should be removed. This is a minor thing though.

I didn't notice the with_lock_retries issue in the down migration, so I'll start looking for that as well.

It would've been fine to leave the with_lock_retries, I normally suggest removing it if it's not required because someone else can use that migration as an example and also use with_lock_retries when it's not necessary.

DB testing: Simplify migration observers : gitlab-org/gitlab!68098 (merged)

During review:

• This didn't have any queries in it, but I noticed a place where we could use a local variable instead of an instance variable.

Post-review:

• Merged as-is.

@abrandl Please add feedback, and compare this review to the average maintainer review.

Remove a n+1 situation in the NPM presenter : gitlab-org/gitlab!68275 (merged)

During review:

• I recommended a modified index that would improve a query plan.
• I recommended removing columns from an index when those columns were contained in the where clause of the index and constrained to a constant.
• I saw a pattern of the form parents.preload(:children).find_each { |p| do_something(p.children.last) } and asked about loading only the last child for each parent record.

Post-review:

• @tigerwnz pointed out a place elsewhere in the codebase where we use a lateral join to solve the "preload 1 child per parent" pattern. I hadn't recommended a lateral join because I thought it would be difficult to integrate with ActiveRecord.

@tigerwnz Please add feedback, and compare this review to the average maintainer review.

Add the retry button to failed migrations : gitlab-org/gitlab!67504 (merged)

During review:

• I recommended that a transaction running over each failed job be split to a smaller transaction for each batch of 100 failed jobs, so that the runtime was bounded per-transaction.

• I recommended locking a set of jobs in aggregate, rather than one at a time.

Post-review:

• Merged as-is.

@abrandl Please add feedback, and compare this review to the average maintainer review.

Database work to support inherited templates on service desk: gitlab-org/gitlab!67546 (merged)

During review:

• I suggested removing a with_lock_retries helper since a table wasn't high-traffic

Post-review:

• @abrandl recommended making the index WHERE file_template_project_id IS NOT NULL to reduce its size. This makes a lot of sense to me as most service desk settings won't have one set.

@abrandl Please add feedback, and compare this review to the average maintainer review.

Setup GitLab MailGun endpoint for syncing bounced invite emails - redo2 : gitlab-org/gitlab!68307 (merged)

During review:

• I had no comments

Afte review:

• Merged as-is

@tigerwnz Please add feedback, and compare this review to the average maintainer review.

Handle database connection errors without causes : gitlab-org/gitlab!68858 (merged)

During review:

• I had no comments

After review:

• Merged as-is

@pbair Please add feedback, and compare this review to the average maintainer review

Update list of forbidden tables when creating indexes : gitlab-org/gitlab!68913 (merged)

During review:

• I suggested a typo fix

After review:

• Merged as-is

@abrandl Please add feedback, and compare this review to the average maintainer review

Add Geo replication for Pages (no verification) : gitlab-org/gitlab!68662 (merged)

During review:

• I asked a few questions because I was unfamiliar with how Geo works in general.
• I suggested a simpler version of the migration, but the author decided to stick with the migration version in the geo replication template

After review:

• @mayra-cabrera agreed with my simpler migration suggestion, and it was applied.

@mayra-cabrera Please add feedback, and compare this review to the average maintainer review

I suggested a simpler version of the migration, but the author decided to stick with the migration version in the geo replication template

Good suggestion! Removing complexity where it's not needed makes the migration more readable and maintainable.

Add option to send slack notifications when a new vulnerability is detected : gitlab-org/gitlab!65245 (merged)

During review:

• I suggested a way to simplify a migration
• I asked a quesiton about where we were storing the slack data, since I saw a slack_integrations table. @toupeira explained why we weren't using it here.

After review:

• Merged as-is

@abrandl Please add feedback, and compare this review to the average maintainer review

Add sorting to integrated error tracking : gitlab-org/gitlab!68777 (merged)

During review:

• I suggested adding indexes to match each sorting pattern that the UI exposed.

After review:

• @abrandl and I discussed the tradeoff between having a "perfect index" for each query and having a single, "good enough" index. I had originally assumed that due to gitlab's scale we can't tolerate any amount of table scans and need close to perfect indexes for every query, but @abrandl showed me that disk access is actully much quicker than I was estimating in my head, and infrequently scanning and sorting a few thousand rows really isn't a problem. After this MR I started considering good-enough indexes much more seriously.

@abrandl Please add feedback, and compare this review to the average maintainer review

Add locked column to Ci::JobArtifact : gitlab-org/gitlab!68893 (merged)

During review:

• There was a lot of discussion in this review around how to keep the column from Ci::Pipeline in sync with the one in Ci::JobArtifact. The author's original plan was to use a trigger, and use one of the valid values of Ci::Pipeline.locked as the default. I suggested not using a trigger, as we almost never use them in the gitlab code base. @fabiopinoto recommended defaulting to an unknown value which I also liked.

After review:

• Merged as-is

@tigerwnz Please add feedback, and compare this review to the average maintainer review

Replace hardcoded database config name :main : gitlab-org/gitlab!69551 (merged)

During review

• I had no comments

After review

• Merged as-is

@pbair Please add feedback, and compare this review to the average maintainer review

Migration to swap ci_builds.stage_id column : gitlab-org/gitlab!66688 (merged)

During review

• I had no comments

After review

• Merged as-is

@krasio Please add feedback, and compare this review to the average maintainer review

Nothing to add, seamless experience, will review again.

Resolve "Address the Primary Key Overflow risk for the ci_build_needs table - Step 3: Drop temporary columns and triggers" : gitlab-org/gitlab!69473 (merged)

During review

• I had no comments

After review

• @pbair pointed out that a line in this MR was duplicated from another one, and that there was a merge conflict.

@pbair Please add feedback, and compare this review to the average maintainer review

I think I only noticed the line was duplicated because I had also reviewed the previous MR. Otherwise, I wouldn't expect anyone to know that, because there was no indication.

Set job_token_scope_enabled to false by default : gitlab-org/gitlab!69502 (merged)

During review

• I suggested changes to this migration. It was originally running a single update across the entire table. Instead I suggested switching it to use batches.
• Even in batches, this migration took longer than the 10 minute timeout limit for post-deploy data migrations. The batch size was too small for most of the rows, which did not need to be updated, but too large for the few rows that did need to be updated. I suggested nesting the batching with different batch sizes, so that the bulk of rows that did not need to be updated could be processed much more quickly. This greatly reduced the migration's runtime. gitlab-org/gitlab!69502 (comment 671610833) has more details.

After review

• I missed that the data migration didn't have disable_ddl_transaction!, which it obviously needed. I've opened gitlab-org/database-team/gitlab-com-database-testing#33 (closed) as a first step towards making the database testing pipeline warn when the migration has a long-running transaction, so that this can't be missed during a future review.

@tigerwnz Please add feedback, and compare this review to the average maintainer review

Migration to finalize swap of bigint ci_builds.id : gitlab-org/gitlab!65201 (merged)

During review:

• I mentioned coordinating a foreign key rename with another MR

After review:

• @abrandl followed up on the foreign key comment by renaming it in the MR

@abrandl Please add feedback, and compare this review to the average maintainer review

Retain non-empty partitions : gitlab-org/gitlab!69850 (merged)

During review

• I had no comments

After review

• Merged as-is

@abrandl please add feedback, and compare this review to the average maintainer review

Resolve "Remove rubocop rule preventing indexes from being added to tables: ci_builds_metadata, events, taggings" : gitlab-org/gitlab!70050 (merged)

During review:

• I had no comments

After review:

• @krasio mentioned that the commit shouldn't have a changelog entry

@krasio please add feedback, and compare to the average maintainer review

Use SafeRequestStore in the DB LB : gitlab-org/gitlab!70003 (merged)

During review:

• I wasn't convinced that this change actually solved the flaky spec it was referencing, so I applied the MR's diff on top of the flaky spec, and saw that it still failed.
• I approved the MR as still a good idea, but not a complete solution to the problem it was trying to solve

After review:

• Merged as-is

@sabrams please add feedback, and compare to the average maintainer review

Remove custom getter methods for secondary emails : gitlab-org/gitlab!69648 (merged)

During review:

• I asked a question about allow_nil vs allow_blank and mentioned that we might want to eventually clean up a few hundred values that have empty strings but should be null

After review:

• Merged as-is

@mayra-cabrera please add feedback, and compare to the average maintainer review

Sadly this one caused a production incident gitlab-com/gl-infra/production#5549 (closed). Spotting these problems at the code-review stage is always difficult, and normally database reviews on migrations and performance problems. Still, it's a good idea to keep an eye on the production incidents as it gives you a perspective of the infra problems GitLab.com faces.

Add setting to toggle Suggest Pipeline feature : gitlab-org/gitlab!70756 (merged)

During review:

• I had no comments

After review:

• @sabrams noticed that the structure.sql file had reordered an unrelated column. I've started looking more closely at the file for future reviews.

@sabrams please add feedback, and compare to the average maintainer review

Add project setting: prevent self-merge on protected branches : gitlab-org/gitlab!70664 (closed)

During review:

• I mentioned that it's OK to add a non-null column if it has a default starting in postgres 11.
• I asked for a setting to be moved from projects to project_settings
• I made some general suggestions around our migration format guidelines

After review:

• Maintainer had no comments
• This community contribution ended up not being accepted - see gitlab-org/gitlab!70664 (comment 684552891) but I'm including it here since it passed through DB maintainer review.

@sabrams please add feedback, and compare to the average maintainer review

Remove foreign key from terraform_state_versions to ci_builds : gitlab-org/gitlab!70743 (merged)

During review:

• I had no comments

After review:

• @krasio pointed out that we should move the migration to a post migration, since the current app code is relying on the foreign key that it is removing. By moving to a post migration, the code will stop relying on the foreign key before we run the migration. I'll start paying more attention to the three states the application can be in - migrations have run, old code / migrations have run, new code / migrations and post migrations have run, new code.

@krasio please add feedback, and compare to the average maintainer review

Not much to add other than what you said above - we should keep in mind at what point of the deployment lifecycle will the migration be executed, and how it may affect the app.

Remove [analyzer]_pipeline metrics join on ci_builds : gitlab-org/gitlab!71210 (merged)

During review:

• I learned a lot about how the hyperloglog algorithm works and gitlab's implementation of it
• I had no comments

After review:

• @mayra-cabrera mentioned that the changed query was still a bit slow with cold caches, but that it was an acceptable improvement. I had noticed this too and thought the same thing, but didn't mention it. We could add an index to speed it up by allowing an index-only scan, but I didn't think it was worth the index maintenance cost, especially since this is part of usage ping, which has other, much slower operations.

@mayra-cabrera please add feedback, and compare to the average maintainer review.

We could add an index to speed it up by allowing an index-only scan, but I didn't think it was worth the index maintenance cost, especially since this is part of usage ping, which has other, much slower operations.

Yeah an index might be overengineering since usage ping is executed only once a week, and it includes heavy operations as you mentioned. For historic purposes, I think it's a good idea to mention when a query is not performing according to our standards, but it's acceptable given the context in which performs. This could help understand newcomers or people unfamiliar with the codebase why such query was introduced in the first place.

Add ON DELETE constraint to security_policy_management_project_id : gitlab-org/gitlab!71391 (merged)

During review:

• I suggested a much simpler migration at the cost of running with 2 foreign keys between the same pair of columns (with different ON DELETE conditions) during the migration. The table involved has less than 100 rows, so I didn't expect it to cause too much load on the database.

After review:

• Maintainer had no comments

@pbair please add feeddback, and compare to the average maintainer review.

Fix default values for the deprecated API throttle : gitlab-org/gitlab!71449 (merged)

During review:

• I suggested removing with_lock_retries! since the involved table is not high-traffic.
• I suggested a slightly simpler migration with change instead of up / down

After review:

• Maintainer had no comments

@krasio please add feedback, and compare to the average maintainer review

Rerun ConvertStringifiedRawMetadataHashToJson m... (gitlab-org/gitlab!80768 - merged)

During the review:

• I used in-progress tooling from the database testing project to uncover that the wrong constant name was used when scheduling the initial migration, and to verify that the migration would behave correctly if rescheduled with the correct constant name.

After review:

• There was a small discussion about adding track_jobs: true to the migration. I didn't want to change the already-reviewed content of the migration that we were rerunning, so I hadn't asked for this change before.

@krasio please add feedback, and compare to the average maintainer review.

Nice catch about the wrong constant name!

The track_jobs: true discussion was more about me getting some context around this MR.

feat: add queries and methods to calculate the ... (gitlab-org/container-registry!903 - merged)

During the review:

• I noticed that indexes need text_pattern_ops to support text prefix searches
• I found 2 places where by only joining one half of a composite primary key, we were inadvertently causing seq scans. Previously it was expected that these seq scans were happening because the container registry database is so small, but I verified that the indexes were ineligible by pulling a plan with enable_seqscan=off in database lab.
• I found that postgres cannot use a prefix index for queries of the form where column like concat('literal', $1), likely because it chooses indexes before optimizing out constant function calls. I recommended computing the concatenation before injecting the parameter for better performance.

As this MR is for the gitlab-org/container-registry project, there wasn't a maintainer to follow my review.

Check for ancestors confidential access in Epic... (gitlab-org/gitlab!79236 - merged)

During review:

• I had no comments After review:
• Maintainer recommended an additional spec.
• Maintainer noticed that query plans had been run with a warm cache, and requested that they be run with a cold cache. Good catch!

@Alexand please add feedback, and compare to the average maintainer review.

@stomlinson,

I liked that you added your opinion on other person's thread. It shows that you were assessing the MR as a whole. I also liked that you reviewed the MR in good timing.

The cold/warm cache thingy is easy to miss. I also missed it on my first pass. But indeed I've seen already many MRs where developers forget or didn't know about it. So it's a good thing to have it on your checklist, to maybe just start quickly looking into the explain's shared buffers reads:.

Keep up the good work!

Backfill project namespaces for specific group (gitlab-org/gitlab!73640 - merged)

During review:

• I pointed out that the default background migration batching strategy wasn't appropriate for this background migration, and recommended a custom one to exploit an index on namespaces - gitlab-org/gitlab!73640 (comment 791986346)
• I suggested moving a query that synced namespace ids for new namespaces into the transaction that created the namespaces.
• I noticed that a gin index pending list clear could happen during a transaction that was explicitly LOCKing project records. By inserting explicit calls to gin_clean_pending_list, we were able to move this work outside the transaction, greatly reducing the duration of the lock. - gitlab-org/gitlab!73640 (comment 814321504)

After review:

• Merged without comment

@abrandl please add feedback, and compare to the average maintainer review.

Add extra validation in corpus model (gitlab-org/gitlab!74370 - merged)

During review:

• I recommended removing an index existence check that was only there for local development, to make the migration simpler.
• I asked that an auto generated migration comment be removed

After review

• Maintainer asked that a delete statement only there for local development also be removed
• Maintainer had a question about replacing one index with another, including a short period when neither would exist.

I had seen the delete, but assumed that there were a few rows to delete, rather than it being unnecessary. After finding the extraneous index check, I should have doubled back and re-considered my assumption.

I hadn't been worried about the index replacement problem, since there was a comment in the migration indicating that the table was accessed behind a disabled feature flag. Looking back at this migration, I wish I had considered asking about how this migration could behave if it was run by a self-managed instance after the feature was toggled on.

@mayra-cabrera please add feedback, and compare to the average maintainer review.

mentioned in merge request !101722 (merged)

marked the checklist item Send one MR to improve the review documentation as completed

marked the checklist item Get yourself added to @gitlab-org/maintainers/database group (this group is recognized in CODEOWNERS). Reach out to any existing maintainer. as completed

marked the checklist item Create a merge request updating your team member entry using the template proposing yourself as a database maintainer, assigned to your manager. as completed

With !101722 (merged) merged, I think this can be closed!

closed