Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for materialized CTEs #4066

Merged
merged 6 commits into from Dec 20, 2022

Conversation

maartenvanvliet
Copy link
Contributor

@maartenvanvliet maartenvanvliet commented Dec 19, 2022

Adds support for MATERIALIZED CTE's. Before Postgres 12 CTE's were materialized by default but from then on it was opt-in with the MATERIALIZED keyword. In some cases this can be beneficial because then the CTE can act as a optimisation fence, which can result in better query plans sometimes.

This PR still needs some work, so I'm opening it to see if there's interest in this. If so, have I taken the right approach here?

Corresponding echo_sql PR elixir-ecto/ecto_sql#466

See also https://www.postgresql.org/docs/current/queries-with.html#id-1.5.6.12.7 and https://paquier.xyz/postgresql-2/postgres-12-with-materialize/

lib/ecto/query.ex Outdated Show resolved Hide resolved
Copy link
Member

@greg-rychlewski greg-rychlewski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something is a bit off, some of the tests that should be there are not in your branch. Like this one in the planner test:

   test "with CTEs" do
      cte_query = from(s in "schema", select: %{x1: selected_as(s.x, :integer), x2: s.x})

      %{with_ctes: %{queries: [{"schema_cte", inner_query}]}} =
        Comment
        |> with_cte("schema_cte", as: ^cte_query)
        |> normalize()

      assert [{:integer, _}, {:x2, _}] = inner_query.select.fields
    end

By any chance is your fork out of date? would you be able to rebase it?

lib/ecto/query/builder/cte.ex Show resolved Hide resolved
Copy link
Member

@greg-rychlewski greg-rychlewski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me thank you. I'll just let @v0idpwn or @josevalim approve as well. Then once this is merged we can point your ecto_sql PR back to it.

Copy link
Member

@v0idpwn v0idpwn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just 1 question and 1 small fix

lib/ecto/query.ex Outdated Show resolved Hide resolved
lib/ecto/query.ex Outdated Show resolved Hide resolved
@greg-rychlewski greg-rychlewski merged commit 954eda4 into elixir-ecto:master Dec 20, 2022
4 of 7 checks passed
@maartenvanvliet
Copy link
Contributor Author

Thx everyone! Happy holidays! 🎅

josevalim added a commit that referenced this pull request Dec 20, 2022
This reverts commit 42358c2.

It should apply to v3.10 only.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants