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

Exposed method to control the readonly query parameter #181

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

serg-temchenko
Copy link

@serg-temchenko serg-temchenko commented Nov 25, 2024

Summary

This PR is meant to improve the control over the readonly option sent by the client to the CH database. Right now, it is defined by some methods like Query::do_execute, but most of the public methods like Query::fetch, Query::fetch_one, or Query::fetch_all don’t have control over this option. This makes it impossible to query the database with queries that contain a lot of columns and require temporary table creation on the database side, which results in the following error:

Code: 164. DB::Exception: db_user: Cannot execute query in readonly mode. (READONLY) (version 24.10.1.2812 (official build))

By adding the changes in this PR, we will gain control over this option, making it possible to request data that requires the creation of temporary tables by CH.

Checklist

  • A human-readable description of the changes was provided so that we can include it in CHANGELOG later

@CLAassistant
Copy link

CLAassistant commented Nov 25, 2024

CLA assistant check
All committers have signed the CLA.

@mshustov mshustov requested a review from slvrtrn November 26, 2024 09:14
@@ -58,7 +60,7 @@ impl Query {

/// Executes the query.
pub async fn execute(self) -> Result<()> {
self.do_execute(false)?.finish().await
self.do_execute()?.finish().await
Copy link
Contributor

Choose a reason for hiding this comment

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

As it is now set as true by default, won't this fail with DDLs now?

Copy link
Contributor

@slvrtrn slvrtrn left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution; it makes sense, given that the read-only param is hardcoded by default. Let's fix the logic (see the comment, CI also confirms that), add some more tests regarding enabled/disabled readonly mode, and maybe an example illustrating the use case.

NB: failing MSRV CI check is supposed to be fixed after #180

@slvrtrn
Copy link
Contributor

slvrtrn commented Nov 26, 2024

At a second glance, would it make more sense just to make it so that .with_option("readonly," "0") overrides the default hardcoded read-only param? In that case, we don't need to have a separate method for that, it will be consistent with other settings usage, and we can just document with_option usage via an example.

@slvrtrn
Copy link
Contributor

slvrtrn commented Nov 27, 2024

CC @loyd @serprex have a look please ^

@serg-temchenko
Copy link
Author

serg-temchenko commented Nov 28, 2024

Hey @slvrtrn.

would it make more sense just to make it so that .with_option("readonly," "0") overrides the default hardcoded read-only param?

Sure, I can check/extend tests and docs. But I’m just thinking if overwriting with 0 is the proper way to go, I mean including the readonly option all the time, since afaik it can be configured on the DB side. Basically, the user can be configured with the required access level. It will just limit the client and require handling that on both sides, which is a bit redundant and needs additional maintenance. But, as long as I can perform SELECT queries without encountering confusing exceptions, I'm okay with that :)

Based on this code with hardcoded flag and this PR, I assume it will not overwrite it by default if it is already present in the options.

@slvrtrn
Copy link
Contributor

slvrtrn commented Dec 3, 2024

Perhaps we can just remove readonly setting, which is enforced by default; IIRC, no other language client does that. Maybe this approach will be even more flexible: #184 (readonly can still be attached using with_option)

@serg-temchenko
Copy link
Author

Sure, this will work as well. Thanks.

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.

3 participants