loyd
Always use POST for queries; disable readonly setting by default
Summary
See https://github.com/ClickHouse/clickhouse-rs/pull/181; it is better not to enforce readonly by default at all.
From the docs:
Setting readonly = 1 prohibits the user from changing settings.,
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 attach readonly setting when needed using with_option.
Checklist
- [ ] Unit and integration tests covering the common scenarios were added
- [x] A human-readable description of the changes was provided so that we can include it in CHANGELOG later
- [ ] For significant changes, documentation in README and https://github.com/ClickHouse/clickhouse-docs was updated with further explanations or tutorials
perhaps it is better not to enforce readonly by default at all.
We should clarify why: Setting readonly = 1 prohibits the user from changing settings., which might be a huge deal for ClickHouse users since ClickHouse has 700+ configurable settings. see https://clickhouse.com/docs/en/operations/settings/permissions-for-queries#readonly
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.
as long URLs could be truncated by the proxies.
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. Not sure whether someone uses it.
This is a breaking change
Why (if CH accepts such queries even for restricted users)?
@loyd
This is a breaking change
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)... Shall we proceed with merging this? WDYT?
Shall we proceed with merging this? WDYT?
Yes, but I'm not sure we won't break something unintentionally and will force people to set read_only=1 for most queries.
To be sure, we should update our CI tests to run against restricted (RO) users.
@mshustov
We should clarify why: Setting readonly = 1 prohibits the user from changing settings., which might be a huge deal for ClickHouse users since ClickHouse has 700+ configurable settings. see https://clickhouse.com/docs/en/operations/settings/permissions-for-queries#readonly
I double-checked
When set to 1, allows:
- All types of read queries (like SELECT and equivalent queries).
- Queries that modify only session context (like USE).
So, queries like this will work:
curl -XPOST "http://localhost:8123?readonly=1&default_format=JSONEachRow&max_execution_time=0.5" --data-binary "select * from foo"
{"i":42}
{"i":144}
See that it has
default_format=JSONEachRow max_execution_time=0.5
Still need to clarify what could be a potential corner case with readonly=1...
@slvrtrn, ok, it's fine.
Let's release it as 0.14 after merging neighboring obviously non-breaking PRs (chrono, written_bytes, row(crate))
Closing it for now. Let's discuss it in: https://github.com/ClickHouse/clickhouse-rs/issues/230