Support AS MATERIALIZED in PG12 [RUN AS-IF-FOSS]
What does this MR do?
Conditionally change all CTE query when PG12 is used.
From:
WITH cte AS (
SELECT X
) SELECT Y
To:
WITH cte AS MATERIALIZED (
SELECT X
) SELECT Y
Context (from the issue)
With PostgreSQL 12, CTE behavior changes. Before, CTEs used to be an optimization fence. We've used this sometimes to avoid a poor choice of plans. In PostgreSQL 12, CTEs are not being materialized or used as an optimization fence by default anymore.
In order to keep the same characteristics on PostgreSQL 12, we'll need to mark CTEs with MATERIALIZE
option.
How
This MR introduces a new Arel
node which generates the AS MATERIALIZED
SQL string. The generation of the MATERIALIZED
string is configurable so later we can fine-tune our queries. If the DB version is PG12, the MATERIALIZED
keyword will be added automatically.
Additional changes:
- Update raw CTE queries to optionally use
MATERIALIZED
.
Since adding MATERIALIZED
is depending on the DB connection (check DB version) some String
constants had to be moved to instance methods.
We can clean this up in 14.1.
Testing with GDK
Run a PG 12 docker container in a terminal window:
docker run --rm -it --name postgres12test -p 6000:5432 -e POSTGRES_USER=john -e POSTGRES_PASSWORD=snow -e POSTGRES_DB=gitlab postgres:12-alpine
Load the schema:
PGUSER=john PGPASSWORD=snow PGHOST=localhost PGPORT=6000 rake db:structure:load
Note: I got an error here, but the schema was actually loaded successfully.
Start a rails console:
rails c
Run:
g = FactoryBot.create(:group)
g2 = FactoryBot.create(:group, parent: g)
puts g.self_and_descendants.to_sql
cte = Gitlab::SQL::CTE.new(:test, User.active)
puts User.with(cte.to_arel).to_sql
You should see AS MATERIALIZED
in the queries.
Does this MR meet the acceptance criteria?
Conformity
-
Does this MR need a changelog?-
I have included a changelog entry. -
I have not included a changelog entry because _____.
-
-
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
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
Related to #245323 (closed)
Merge request reports
Activity
changed milestone to %13.10
added backend label
2 Warnings There was a problem trying to check the Changelog. Exception: NoMethodError - undefined method `empty?' for 56976:Integer This MR has a Changelog file outside ee/
, but code changes inee/
. Consider moving the Changelog file intoee/
.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.
The following files require a review from the Database team:
db/migrate/20200609212701_add_incident_settings_to_all_existing_projects.rb
db/post_migrate/20191105094625_set_report_type_for_vulnerabilities.rb
db/post_migrate/20191114173624_set_resolved_state_on_vulnerabilities.rb
db/post_migrate/20200305082754_remove_duplicate_labels_from_project.rb
db/post_migrate/20200716234259_remove_duplicate_labels_from_group.rb
db/post_migrate/20200809221641_migrate_license_management_artifacts_to_license_scanning.rb
db/post_migrate/20200831065705_ensure_target_project_id_is_filled.rb
db/post_migrate/20210105030125_cleanup_projects_with_bad_has_external_wiki_data.rb
db/post_migrate/20210210221006_cleanup_projects_with_bad_has_external_issue_tracker_data.rb
ee/db/geo/migrate/20170906174622_remove_duplicates_from_project_registry.rb
lib/gitlab/background_migration/backfill_design_internal_ids.rb
lib/gitlab/background_migration/backfill_project_updated_at_after_repository_storage_move.rb
lib/gitlab/background_migration/copy_merge_request_target_project_to_merge_request_metrics.rb
lib/gitlab/background_migration/fix_projects_without_project_feature.rb
lib/gitlab/background_migration/fix_projects_without_prometheus_service.rb
lib/gitlab/background_migration/fix_user_namespace_names.rb
lib/gitlab/background_migration/fix_user_project_route_names.rb
lib/gitlab/background_migration/populate_has_vulnerabilities.rb
lib/gitlab/background_migration/wrongfully_confirmed_email_unconfirmer.rb
lib/gitlab/database/as_with_materialized.rb
lib/gitlab/database/bulk_update.rb
lib/gitlab/database/postgres_hll/batch_distinct_counter.rb
lib/gitlab/sql/cte.rb
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 Ethan Urie ( @eurie
) (UTC-4, 5 hours behind@ahegyi
)Rémy Coutable ( @rymai
) (UTC+1, same timezone as@ahegyi
)database Alex Ives ( @alexives
) (UTC-5, 6 hours behind@ahegyi
)Mayra Cabrera ( @mayra-cabrera
) (UTC-6, 7 hours behind@ahegyi
)~migration No reviewer available No maintainer available If needed, you can retry the
danger-review
job that generated this comment.Generated by
DangerEdited by 🤖 GitLab Bot 🤖added 1 commit
- 7ca7ee7f - Update queries to use MATERIALIZED when supported
removed [deprecated] Accepting merge requests label
changed milestone to %13.11
added missed:13.10 label
mentioned in issue #245323 (closed)
added 105 commits
-
7ca7ee7f...8de1bc8a - 104 commits from branch
master
- 3816ef44 - Support AS MATERIALIZED in PG12
-
7ca7ee7f...8de1bc8a - 104 commits from branch
- Resolved by Michael Kozono
Hi @minac ,
Could you do the BE review?
Edited by Adam Hegyi
- Resolved by Adam Hegyi
Hi @iroussos,
Could you do the initial DB review?
- Resolved by Michael Kozono
Hi @mkozono,
Could you check if the query changes affect Geo in any way? Did I miss something?
requested review from @mkozono
- Resolved by Adam Hegyi
- Resolved by Adam Hegyi
- Resolved by Adam Hegyi
added databasereviewed label and removed databaseactive label
added databaseactive workflowin review labels and removed databasereviewed workflowready for development labels
added typefeature typemaintenance labels
- Resolved by Adam Hegyi
mentioned in issue #325639 (closed)
- Resolved by Adam Hegyi
added 492 commits
-
d7c7bb81...6bfffffc - 491 commits from branch
master
- 04c11221 - Support AS MATERIALIZED in PG12
-
d7c7bb81...6bfffffc - 491 commits from branch
enabled an automatic merge when the pipeline for 4857160b succeeds
mentioned in commit 9c0ef184
mentioned in issue #325916 (closed)
added releasedcandidate label
mentioned in issue #331089 (closed)
mentioned in merge request !63841 (merged)