-
Notifications
You must be signed in to change notification settings - Fork 98
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
Always use POST for queries; disable readonly setting by default #184
base: main
Are you sure you want to change the base?
Conversation
We should clarify why: |
Co-authored-by: Philip Dubé <[email protected]>
As far as I remember, (older?) CH version rejected some SELECT-over-POST queries without readonly=1 if a user is restricted. But it could be my false memory or a bug in older versions.
On the other hand, providers can log these queries or even produce metrics, but this is usually done only for query params. This is why, initially, this crate used the SELECT-over-GET approach.
Why (if CH accepts such queries even for restricted users)? |
Perhaps not breaking, indeed, only for the very outdated ClickHouse versions (which we don't officially support anyway)... |
Yes, but I'm not sure we won't break something unintentionally and will force people to set To be sure, we should update our CI tests to run against restricted (RO) users. |
I double-checked
So, queries like this will work:
See that it has
Still need to clarify what could be a potential corner case with |
@slvrtrn, ok, it's fine. Let's release it as |
Summary
See #181; it is better not to enforce
readonly
by default at all.From the docs:
This might be a huge deal for ClickHouse users since ClickHouse has 700+ configurable settings. Currently, a query with length > 8192 that fetches data will not be allowed to change settings because of forced
readonly=1
in that code branch.Additionally, it is possibly safer to just always send queries via the POST body, as long URLs could be truncated by the proxies.
This is a breaking change (
readonly
setting is no longer set by default for queries that fetch data); however, it is still possible to attachreadonly
setting when needed usingwith_option
.Checklist