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 EXPLAIN statement #231

Merged
merged 30 commits into from
Jun 7, 2020

Conversation

leandrocp
Copy link
Contributor

@leandrocp leandrocp commented May 24, 2020

Please see elixir-ecto/ecto#3313

Considering it's just a draft and the implementation isn't complete, the CI is gonna fail but it was tested against Postgres 12:

% export ECTO_PATH=../ecto
% export ECTO_ADAPTER=pg
% mix test integration_test/sql/sql.exs:131
Excluding tags: [:test]
Including tags: [line: "131"]

.

Finished in 0.1 seconds
17 tests, 0 failures, 16 excluded

Randomized with seed 5905

It's still a WIP to gather feedback
@josevalim
Copy link
Member

Looks great! I have also dropped a comment on the Ecto issue, as I think this could be totally implemented on EctoSQL and not touch Ecto. Also, will we simply make sure the explain results are printable so people do IO.puts Repo.explain? Or should we convert them to some special data structure?

@fxn I believe you implemented this for ActiveRecord, right? Do you have any words of wisdom to share? :)

@leandrocp
Copy link
Contributor Author

this could be totally implemented on EctoSQL and not touch Ecto

I'll update the PR for your review. 👍

will we simply make sure the explain results are printable so people do IO.puts Repo.explain? Or should we convert them to some special data structure?

Postgres and Mysql support the following formats, respectively:

# postgres
FORMAT { TEXT | XML | JSON | YAML }
# mysql
format_name: {
    TRADITIONAL
  | JSON
  | TREE
}

I don't think a "special data structure" (a specific Struct for example) is needed but it could be String.t | map() to support those formats.

@josevalim
Copy link
Member

Oh, good to know. Let's start with TEXT and we will see how to support multiple formats later. :)

@fxn
Copy link

fxn commented May 25, 2020

Hey!!!

Sure! Let me share some things I recall from those days:

  • EXPLAIN seemed too database-specific to be generalizable. Since its main use case was to print and read directly from AR instead of doing all the dance getting the SQL and going to the database shell, as you guys say above, I also chose to go with a textual return value. Once that decision was taken, I mimicked the result of the shells of the supported databases as pixel-perfect as possible 😎. As a user you even forget you are in a Rails console. See pretty-printing for MySQL, PostgreSQL, and SQLite.

  • I do not know if this is relevant for Ecto, but in Active Record one API call could result in several SQL statements executed behind the scenes for performance. I believe the main use case was eager loading associations. That was implementation. From the point of view of the user, the return value was the expected one for that call, AR collected whatever had to collect to present it that way. However, that meant the EXPLAIN feature could not assume one call => one statement. The only sensible choice I saw was to program this in a way that captured all executed statements, and run EXPLAINs for each one of them. Most of the time, you got one anyway.

  • The initial feature provided automatic EXPLAIN too. If any query in development took more than a certain threshold, you got a EXPLAIN logged automatically. As time passed, we removed the automatism because in development you normally don't have the production workload, or even the data set. It was less helpful than we thought. Nowadays, you only have the ability to print on demand, as proposed in this patch.

  • Since the console was the main use case, and IRB calls inspect on the value of the entered expression and prints it, we overrode inspect in the returned string. Otherwise, you had to > puts query.explain; 1 or something to get a readable output. Too impractical, this is a very niche feature let's say, so simply evaluating > query.explain prints a readable output as users expect.

There goes some feedback. Cool that Ecto is getting this too!

@josevalim
Copy link
Member

Thank you @fxn! This is very helpful! Some notes based on your comments:

  • Regarding the multiple queries, I think our explain will discard the data preloads, at least for v1

  • The automatic explains sound interesting... but noted on the development case. Maybe this is something we can integrate with the Phoenix Live Dashboard? You can enable auto-explain with the RequestLogger or similar. So it is on demand and you can easily simulate production workloads

lib/ecto/adapters/sql.ex Outdated Show resolved Hide resolved
@leandrocp
Copy link
Contributor Author

leandrocp commented May 26, 2020

Hey @fxn thanks for the feedback!

Guys here's a new commit to check if I'm going into the right direction: https://github.com/elixir-ecto/ecto_sql/pull/231/commits/f28c8423e7752623658fff5f8abefa7d99aae2b9 - the idea is pretty simple, each adapter needs to implement its own explain_query taking the resolved query and opts as arguments, and that return will be executed in a transaction to avoid side effects. Also please see comments and TODOs in that commit.

lib/ecto/adapters/sql.ex Outdated Show resolved Hide resolved
@leandrocp
Copy link
Contributor Author

leandrocp commented May 26, 2020

Maybe this is something we can integrate with the Phoenix Live Dashboard?

@josevalim explain could add some measurements in the telemetry event. Is that something we could explore on this PR or leave it to another PR?

lib/ecto/adapters/sql.ex Outdated Show resolved Hide resolved
and addressed review comments.

TODO: implement EXPLAIN for tds and myxql
lib/ecto/adapters/sql.ex Outdated Show resolved Hide resolved
Leandro Cesquini Pereira added 3 commits May 29, 2020 08:39
lib/ecto/adapters/sql.ex Outdated Show resolved Hide resolved
lib/ecto/adapters/sql.ex Outdated Show resolved Hide resolved
lib/ecto/adapters/sql.ex Outdated Show resolved Hide resolved
lib/ecto/adapters/sql.ex Outdated Show resolved Hide resolved
lib/ecto/adapters/sql.ex Outdated Show resolved Hide resolved
Co-authored-by: José Valim <jose.valim@gmail.com>
lib/ecto/adapters/sql.ex Outdated Show resolved Hide resolved
lib/ecto/adapters/sql.ex Outdated Show resolved Hide resolved
lib/ecto/adapters/sql.ex Outdated Show resolved Hide resolved
lib/ecto/adapters/sql.ex Outdated Show resolved Hide resolved
lib/ecto/adapters/sql.ex Outdated Show resolved Hide resolved
Leandro Cesquini Pereira and others added 5 commits June 6, 2020 11:57
Co-authored-by: José Valim <jose.valim@gmail.com>
Co-authored-by: José Valim <jose.valim@gmail.com>
Co-authored-by: José Valim <jose.valim@gmail.com>
Built-in adapters support passing `opts` to the EXPLAIN statement according to the following:

Adapter | Supported opts | Notes
---------------- | -------------- | -----
Copy link
Member

Choose a reason for hiding this comment

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

Please see if this fixes the docs:

Suggested change
---------------- | -------------- | -----
:--------------- | :------------- | :----

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given this markdown:

Adapter          | Supported opts | Notes
:--------------- | :------------- | :----
MyXQL            | None           | `EXTENDED` and `PARTITIONS`

It generates the correct HTML:

<table><thead><tr><th style="text-align: left;">Adapter</th><th style="text-align: left;">Supported opts</th><th style="text-align: left;">Notes</th></tr></thead><tbody><tr><td style="text-align: left;">MyXQL</td><td style="text-align: left;">None</td><td style="text-align: left;"><code class="inline">PARTITIONS</code> and <code class="inline">EXTENDED</code></td></tr></tbody></table>

But given this:

Adapter          | Supported opts | Notes
:--------------- | :------------- | :----
MyXQL            | None           | `EXTENDED` and `PARTITIONS` opts

Generates:

<table><thead><tr><th style="text-align: left;">Adapter</th><th style="text-align: left;">Supported opts</th><th style="text-align: left;">Notes</th></tr></thead><tbody><tr><td style="text-align: left;">MyXQL</td><td style="text-align: left;">None</td><td style="text-align: left;"> opts<code class="inline">PARTITIONS</code> and <code class="inline">EXTENDED</code></td></tr></tbody></table>

For some reason the "opts" word in rendered before the inlined codes:

Screen Shot 2020-06-07 at 9 45 52 AM

Using latest ex_doc, running mix compile && ex_doc "ecto_sql", "1", _build/dev/lib/ecto_sql/ebin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like a bug? I can open an issue or take a look on this.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a Earmark bug. Note you can generate the docs like this: MIX_ENV=docs mix docs. Then I would try updating the Earmark version and see if it fixes it!

Copy link
Contributor Author

@leandrocp leandrocp Jun 7, 2020

Choose a reason for hiding this comment

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

Same result using latest versions:

cat mix.lock | grep earmark
  "earmark": {:hex, :earmark, "1.4.5", "62ffd3bd7722fb7a7b1ecd2419ea0b458c356e7168c1f5d65caf09b4fbdd13c8", [:mix], [], "hexpm", "b7d0e6263d83dc27141a523467799a685965bf8b13b6743413f19a7079843f4f"},
  "ex_doc": {:hex, :ex_doc, "0.22.1", "9bb6d51508778193a4ea90fa16eac47f8b67934f33f8271d5e1edec2dc0eee4c", [:mix], [{:earmark, "~> 1.4.0", [hex: :earmark, repo: "hexpm", optional: false]}, {:makeup_elixir, "~> 0.14", [hex: :makeup_elixir, repo: "hexpm", optional: false]}], "hexpm", "d957de1b75cb9f78d3ee17820733dc4460114d8b1e11f7ee4fd6546e69b1db60"},

I found a related open issue pragdave/earmark#335, so I'll just comment there.

lib/ecto/adapters/sql.ex Outdated Show resolved Hide resolved
lib/ecto/adapters/sql.ex Outdated Show resolved Hide resolved
lib/ecto/adapters/sql.ex Outdated Show resolved Hide resolved
Leandro Cesquini Pereira and others added 3 commits June 7, 2020 09:36
Co-authored-by: José Valim <jose.valim@gmail.com>
Co-authored-by: José Valim <jose.valim@gmail.com>
@josevalim josevalim merged commit 927ccbc into elixir-ecto:master Jun 7, 2020
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@josevalim
Copy link
Member

Thank you, amazing work!

@leandrocp
Copy link
Contributor Author

@josevalim I'm gonna work on the TDS support and also to integrate EXPLAIN on LiveDashboard, following your idea:

"Maybe this is something we can integrate with the Phoenix Live Dashboard? You can enable auto-explain with the RequestLogger or similar. So it is on demand and you can easily simulate production workloads"

Sounds good?

@josevalim
Copy link
Member

@leandrocp sounds great! Although I would have to think about how we would implement this without affecting performance. Today each repository emits a different telemetry event, so there is no easy way to "listen" to all of them. Perhaps the best option for now is to open up an issue in the LiveDashboard repo so we can discuss possible approaches.

@leandrocp
Copy link
Contributor Author

@leandrocp sounds great! Although I would have to think about how we would implement this without affecting performance. Today each repository emits a different telemetry event, so there is no easy way to "listen" to all of them. Perhaps the best option for now is to open up an issue in the LiveDashboard repo so we can discuss possible approaches.

Sure, that's something I have been thinking about and will require a discussion before a draft.

zachdaniel pushed a commit to zachdaniel/ecto_sql that referenced this pull request Sep 19, 2020
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.

4 participants