Implements hints for Postgresql #530
Conversation
|
You can write a unit test at test/ecto/adapters/postgres_test.exs that test we generate the proper SQL. :) |
|
@josevalim done |
|
I like the idea of supporting TABLESAMPLE, but this isn't a hint... |
|
It feels a bit off to me too because Postgres pretty famously doesn't use query hints https://wiki.postgresql.org/wiki/OptimizerHintsDiscussion. The only alternative I think is to add a new option like |
|
@v0idpwn @greg-rychlewski other adapters here implement samples through this hint system. It is even in the documentation |
|
Ah ok, then I guess that's that :). |
|
💚 💙 💜 💛 ❤️ |
|
Glad this is finally supported but using the name “hints” because it’s used elsewhere even when it’s not the right term seems like a poor choice. It will likely be hard to find this functionality with documentation, would you ever expect the hints option to be used for tablesample? |
|
@josevalim would you object if I implement this as :sample in ecto instead? I think the point above about discoverability is fair. It could be dead simple just a compile time string so we are not fighting against all the variability across dbs |
|
If naming can be improved I think it’s worth trying to facilitate |
|
@josevalim btw we actually have a bigger issue here. I was looking up how hints are implemented and the tuple allows dynamic hints. Probably the adapter is expected to handle those properly but here it's just being passed through. So we can have SQL injection: one = ~s(1; drop table "schema")
Schema
|> from(hints: ["TABLESAMPLE SYSTEM": one])
|> select([r], r.x)
|> plan()
|> all()
# "SELECT s0.\"x\" FROM \"schema\" AS s0 TABLESAMPLE SYSTEM 1; drop table \"schema\""So we have to at least rollback the tuple version. I think it's worth rolling back the whole thing tbh and thinking how to properly implement static/dynamic samples + possibly the system time thing mentioned above (but i never used it so can't say too much about it atm). |
|
Yeah, but |
|
Let's see what Jose says because I completely agree with the comment from @bbhoss. Also not too sure we want to get into the SQL escaping game if it can be implemented in a better way. |
|
Yeah, I understand that it makes sense, but static string is practically useless in majority of cases, Dynamic string will have the problem of injections too. The best solution I can see is to implement it with TABLESAMPLE SYSTEM($1) |
|
I don't think calling it We could pick another name but it has to be generic and then, would it actually help with discovery? If it won't, isn't it better to stick with "hints", which already works and at least makes sense for some of them? Regarding the SQL part, I am pretty sure it was intentional, otherwise the uses are very limited. Maybe |
|
@josevalim what about safe DSL for hints like one present for |
|
It doesn't scale because there is no standardization whatsoever. Then Ecto will have to know about specific behaviour of each adapter out there. That's why we just allow a string to be given. If we are worried about the interpolation, then we can require |
|
Why doesn't it scale? Developers will write their own |
|
Oh, I understood what you meant by safe DSL now, you mean |
|
The literal in query has the same issue, so I guess we need to call it |
|
The reason why I was partial to |
|
It would be similar to SQL Server using not having |
|
I see. I think we could have a proposal for sampling then. But we most likely want to close the gaps introduced by hints and literals as well. |
Overview
Implements FROM statement hints for postgresql
Use-case
This is useful for
TABLESAMPLEhint in postgresqlhttps://www.postgresql.org/docs/current/sql-select.html#SQL-FROM
For example:
Fixes well-known problem with getting random rows from the table using Ecto
https://elixirforum.com/t/ecto-add-raw-sql-at-end-of-query-or-rather-choose-row-at-random/3386
If this change requires any tests, I am happy to implement them, just show me how.