Commit 28ac36c6 authored by Russell Dickenson's avatar Russell Dickenson

Merge branch 'tc-db-mr-checklist' into 'master'

Move items from database MR template checklist to documentation

See merge request gitlab-org/gitlab!21051
parents d9b29c3f adaa7935
## What does this MR do?
<!--
Describe in detail what your merge request does, why it does that, etc. Merge
requests without an adequate description will not be reviewed until one is
added.
Please also keep this description up-to-date with any discussion that takes
place so that reviewers can understand your intent. This is especially
important if they didn't participate in the discussion.
Make sure to remove this comment when you are done.
-->
Add a description of your merge request here.
## Database checklist
- [ ] Conforms to the [database guides](https://docs.gitlab.com/ee/development/README.html#database-guides)
When adding migrations:
- [ ] Updated `db/schema.rb`
- [ ] Added a `down` method so the migration can be reverted
- [ ] Added the output of the migration(s) to the MR body
- [ ] Added tests for the migration in `spec/migrations` if necessary (e.g. when migrating data)
- [ ] Added rollback procedure. Include either a rollback procedure or description how to rollback changes
When adding or modifying queries to improve performance:
- [ ] Included data that shows the performance improvement, preferably in the form of a benchmark
- [ ] Included the output of `EXPLAIN (ANALYZE, BUFFERS)` of the relevant queries
When adding foreign keys to existing tables:
- [ ] Included a migration to remove orphaned rows in the source table before adding the foreign key
- [ ] Removed any instances of `dependent: ...` that may no longer be necessary
When adding tables:
- [ ] Ordered columns based on the [Ordering Table Columns](https://docs.gitlab.com/ee/development/ordering_table_columns.html) guidelines
- [ ] Added foreign keys to any columns pointing to data in other tables
- [ ] Added indexes for fields that are used in statements such as `WHERE`, `ORDER BY`, `GROUP BY`, and `JOIN`s
When removing columns, tables, indexes or other structures:
- [ ] Removed these in a post-deployment migration
- [ ] Made sure the application no longer uses (or ignores) these structures
/label ~database ~"database::review pending"
......@@ -20,8 +20,8 @@ changes are reviewed, take the following steps:
1. Ensure the merge request has ~database and ~"database::review pending" labels.
If the merge request modifies database files, Danger will do this for you.
1. Use the [Database changes checklist](https://gitlab.com/gitlab-org/gitlab/blob/master/.gitlab/merge_request_templates/Database%20changes.md)
template or add the appropriate items to the MR description.
1. Prepare your MR for database review according to the
[docs](https://docs.gitlab.com/ee/development/database_review.html#how-to-prepare-the-merge-request-for-a-database-review).
1. Assign and mention the database reviewer suggested by Reviewer Roulette.
MSG
......
......@@ -111,7 +111,6 @@ description: 'Learn how to contribute to GitLab.'
### Best practices
- [Merge Request checklist](database_merge_request_checklist.md)
- [Adding database indexes](adding_database_indexes.md)
- [Foreign keys & associations](foreign_keys.md)
- [Single table inheritance](single_table_inheritance.md)
......
# Merge Request Checklist
When creating a merge request that performs database related changes (schema
changes, adjusting queries to optimize performance, etc) you should use the
merge request template called "Database changes". This template contains a
checklist of steps to follow to make sure the changes are up to snuff.
To use the checklist, create a new merge request and click on the "Choose a
template" dropdown, then click "Database changes".
An example of this checklist can be found at
<https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/12463>.
The source code of the checklist can be found in at
<https://gitlab.com/gitlab-org/gitlab/blob/master/.gitlab/merge_request_templates/Database%20changes.md>
......@@ -32,12 +32,10 @@ for review.
### Roles and process
A Merge Request author's role is to:
A Merge Request **author**'s role is to:
- Decide whether a database review is needed.
- If database review is needed, add the ~database label.
- Use the [database changes](https://gitlab.com/gitlab-org/gitlab/blob/master/.gitlab/merge_request_templates/Database%20changes.md)
merge request template, or include the appropriate items in the MR description.
- [Prepare the merge request for a database review](#how-to-prepare-the-merge-request-for-a-database-review).
A database **reviewer**'s role is to:
......@@ -78,15 +76,54 @@ make sure you have applied the ~database label and rerun the
### How to prepare the merge request for a database review
In order to make reviewing easier and therefore faster, please consider preparing a comment
and details for a database reviewer:
In order to make reviewing easier and therefore faster, please take
the following preparations into account.
- Provide queries in SQL form rather than ActiveRecord.
- Format any queries with a SQL query formatter, for example with [sqlformat.darold.net](http://sqlformat.darold.net).
- Consider providing query plans via a link to [explain.depesz.com](https://explain.depesz.com) or another tool instead of textual form.
- For query changes, it is best to provide the SQL query along with a plan *before* and *after* the change. This helps to spot differences quickly.
- When providing query plans, make sure to use good parameter values, so that the query executed is a good example and also hits enough data.
- Usually, the `gitlab-org` namespace (`namespace_id = 9970`) and the `gitlab-org/gitlab-foss` (`project_id = 13083`) or the `gitlab-org/gitlab` (`project_id = 278964`) projects provide enough data to serve as a good example.
#### Preparation when adding migrations
- Ensure `db/schema.rb` is updated.
- Make migrations reversible by using the `change` method or include a `down` method when using `up`.
- Include either a rollback procedure or describe how to rollback changes.
- Add the output of the migration(s) to the MR description.
- Add tests for the migration in `spec/migrations` if necessary. See [Testing Rails migrations at GitLab](testing_guide/testing_migrations_guide.html) for more details.
#### Preparation when adding or modifying queries
- Write the raw SQL in the MR description. Preferably formatted
nicely with [sqlformat.darold.net](http://sqlformat.darold.net) or
[paste.depesz.com](https://paste.depesz.com).
- Include the output of `EXPLAIN (ANALYZE, BUFFERS)` of the relevant
queries in the description. If the output is too long, wrap it in
`<details>` blocks, paste it in a GitLab Snippet, or provide the
link to the plan at: [explain.depesz.com](https://explain.depesz.com).
- When providing query plans, make sure it hits enough data:
- You can use a GitLab production replica to test your queries on a large scale,
through the `#database-lab` Slack channel or through [chatops](understanding_explain_plans.md#chatops).
- Usually, the `gitlab-org` namespace (`namespace_id = 9970`) and the
`gitlab-org/gitlab-foss` (`project_id = 13083`) or the `gitlab-org/gitlab` (`project_id = 278964`)
projects provide enough data to serve as a good example.
- For query changes, it is best to provide the SQL query along with a
plan _before_ and _after_ the change. This helps to spot differences
quickly.
- Include data that shows the performance improvement, preferably in
the form of a benchmark.
#### Preparation when adding foreign keys to existing tables
- Include a migration to remove orphaned rows in the source table **before** adding the foreign key.
- Remove any instances of `dependent: ...` that may no longer be necessary.
#### Preparation when adding tables
- Order columns based on the [Ordering Table Columns](ordering_table_columns.md) guidelines.
- Add foreign keys to any columns pointing to data in other tables, including [an index](migration_style_guide.md#adding-foreign-key-constraints).
- Add indexes for fields that are used in statements such as `WHERE`, `ORDER BY`, `GROUP BY`, and `JOIN`s.
#### Preparation when removing columns, tables, indexes or other structures
- Follow the [guidelines on dropping columns](what_requires_downtime.md#dropping-columns).
- Generally it's best practice, but not a hard rule, to remove indexes and foreign keys in a post-deployment migration.
- Exceptions include removing indexes and foreign keys for small tables.
### How to review for database
......@@ -136,6 +173,7 @@ and details for a database reviewer:
(eg indexes, columns), you can use a [one-off instance from the restore
pipeline](https://ops.gitlab.net/gitlab-com/gl-infra/gitlab-restore/postgres-gprd)
in order to establish a proper testing environment.
- Avoid N+1 problems and minimalize the [query count](merge_request_performance_guidelines.md#query-counts).
### Timing guidelines for migrations
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment