Quietly prepare unprepared query #806
Conversation
sylwiaszunejko
force-pushed
the
prepare_query
branch
from
dba95cb to
8bbdb1f
Compare
sylwiaszunejko
force-pushed
the
prepare_query
branch
from
8bbdb1f to
b035c50
Compare
scylla/src/transport/session.rs
Outdated
| // Needed to avoid moving query and values into async move block | ||
| let query_ref = &query; | ||
| let values_ref = &serialized_values; | ||
| let paging_state_ref = &paging_state; | ||
| let values_size = values.serialized().unwrap().into_owned().size(); |
There was a problem hiding this comment.
.serialized() is an operation that can be potentially costly. It is already computed on line 624, so I suggest using the already computed serialized_values instead.
scylla/src/transport/session.rs
Outdated
| let serial_consistency = prepared | ||
| .config | ||
| .serial_consistency | ||
| .unwrap_or(default_consistency); |
There was a problem hiding this comment.
The prepared statement will have exactly the same config as the original query object, so there is no need to calculate serial consistency again.
sylwiaszunejko
force-pushed
the
prepare_query
branch
from
b035c50 to
2ec216a
Compare
There was a problem hiding this comment.
Apart from moving the preparation into Connection::query, we will also have to handle batches... Batches can consist of multiple statements, both prepared and unprepared. I guess we have to update Connection::batch as well to handle unprepared statements with arguments. We can start with preparing them sequentially in that function, or with some low (fixed?) concurrency.
scylla/src/transport/session.rs
Outdated
| let values_size = serialized_values.size(); | ||
| async move { | ||
| if values_size != 0 { | ||
| let prepared = connection.prepare(query_ref).await?; | ||
| return connection | ||
| .execute_with_consistency( | ||
| &prepared, | ||
| values_ref, | ||
| consistency, | ||
| serial_consistency, | ||
| paging_state_ref.clone(), | ||
| ) | ||
| .await | ||
| .and_then(QueryResponse::into_non_error_query_response); | ||
| } |
There was a problem hiding this comment.
I just realized that maybe it's not the best place for fixing the issue. There is the Connection::query function which, while not being a public API, is used internally by some of our code, and we would like to benefit from better safety there. I think it is better to call prepare in Connection::query directly.
There was a problem hiding this comment.
So maybe we should call prepare in Connection::query_with_consistency, because both Session::query and Connection::query use that.
sylwiaszunejko
force-pushed
the
prepare_query
branch
from
2ec216a to
9026b72
Compare
Before sending an unprepared statement, quietly prepare it on one connection to obtain the information about the bind markers. If the list of values to be bound is empty, we just send it as an unprepared query.
sylwiaszunejko
force-pushed
the
prepare_query
branch
from
9026b72 to
ec03218
Compare
Motivation
See the referenced issue (#800)
What's done
Information about the types of the bind markers is necessary to make the serialization API safe (i.e. so that the user data won't be misinterpreted when sending it to the database). The problem with unprepared statements is that the driver doesn't have a good way to determine column names and types of the bind markers.
Now before sending an unprepared statement, it is quietly prepared on one connection to obtain the information about the bind markers. If the list of values to be bound is empty, we just send it as an unprepared query.
Evaluation
Integration tests are added.
Refs: #800
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.