Skip to content
Snippets Groups Projects

Support AS MATERIALIZED in PG12 [RUN AS-IF-FOSS]

Merged Adam Hegyi requested to merge 245323-arel-support-for-materialized-cte into master

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

Availability and Testing

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)

Edited by Adam Hegyi

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
  • Mehmet Emin INAC
  • Adam Hegyi added 1 commit

    added 1 commit

    • d7c7bb81 - Support AS MATERIALIZED in PG12

    Compare with previous version

  • Michael Kozono approved this merge request

    approved this merge request

  • Nikolay Samokhvalov
  • Yannis Roussos approved this merge request

    approved this merge request

  • Yannis Roussos requested review from @pbair and removed review request for @iroussos

    requested review from @pbair and removed review request for @iroussos

  • added databasereviewed label and removed databaseactive label

  • Michael Kozono
  • Mehmet Emin INAC approved this merge request

    approved this merge request

  • mentioned in issue #325639 (closed)

  • Patrick Bair
  • Adam Hegyi added 492 commits

    added 492 commits

    Compare with previous version

  • Adam Hegyi resolved all threads

    resolved all threads

  • Adam Hegyi enabled an automatic merge when the pipeline for 4857160b succeeds

    enabled an automatic merge when the pipeline for 4857160b succeeds

  • merged

  • Adam Hegyi mentioned in commit 9c0ef184

    mentioned in commit 9c0ef184

  • mentioned in issue #325916 (closed)

  • mentioned in issue #331089 (closed)

  • Max Orefice mentioned in merge request !63841 (merged)

    mentioned in merge request !63841 (merged)

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