-
-
Notifications
You must be signed in to change notification settings - Fork 486
Idea: combine parse, bind and execute in one packet if no prepared statement is used for extended protocol query methods #1017
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
Comments
We need to know how Postgres parsed the statement to construct the bind packet. If you're concerned about overhead of small queries I would recommend preparing them up front which will be faster than preparing them each time either way. |
Thank you for your response. I had the feeling there was some reason for it. |
IIRC the Java client string-interpolates all of its query parameters or something. |
I fully agree: that for small queries, preparing these upfront will be faster than preparing them each time. And the documentation suggests for queries that are reused to create a prepared statement. However, generally, many statements are small and are used once (along with a lot of statements are reused over and over), and quite often these do not have any placeholders and variables to bind, wouldn't it for these be possible in the first place and and if so an improvement to push out the extended query phases of parse, bind and execute in one go? Or is that simply not possible in the current form of the postgres driver, because it still needs the parse result for the bind packet? Of course it's possible using the rust postgres driver to simply use the query_simple impl, but if possible for simple SQL reducing the number of packets is likely to make a huge difference with bigger deployments. |
Without writing a client-side query parser (which I am not interested in doing) we don't have any reliable way to determine if the query has parameters or not. |
Check. That is clear. |
@sfackler is there a likewise reason every extended query protocol request uses a named statement? Using a named statement the minimal number of roundtrips for a query is 3: parse, bind/execute, close. If no named statement for non-prepared statements is used, it can be reduced by one, because it doesn't need close? |
I think that would be possible with a more sophisticated implementation of |
Is there anything you want me to do about that, such as creating a separate issue? When no prepared statement is used, it makes sense to use an unnamed statement. The reduction of roundtrips from 2 to 3 is likely significant especially in clouds where the client might not be physically close to the database. Obviously the close for database side prepared statements as a result of a statement being prepared client side makes sense, which is the current implementation. |
Probably worth making a separate issue, yeah. One complicating factor is that use of the unnamed statement will break when pipelining queries. When that happens, you can end up with "prepare 1, prepare 2, execute 1, execute 2", so the execute 1 step would end up using query 2's statement. That can probably be worked around with some extra state tracking to prevent overwrites of the unnamed statement but I don't know what it would look like off the top of my head. |
Keep in mind as well that statements aren't closed synchronously - instead it just registers the fact that it needs to be closed in the future, and that'll be folded into the next request if there is one pending. |
Introduce a new `query_with_param_types` method that allows to specify Postgres type parameters. This obviated the need to use prepared statementsjust to obtain parameter types for a query. It then combines parse, bind, and execute in a single packet. Related: sfackler#1017, sfackler#1067
Can this issue be closed since #1147 was merged? |
* fix code block * Update lib.rs * Add table_oid and field_id to columns of prepared statements * Simplify Debug impl of Column * Update id types * feat(types): add default derive to json wrapper Adds a Default impl for `Json<T> where T: Default` allowing for other structs to use the wrapper and implement Default. * Update env_logger requirement from 0.10 to 0.11 Updates the requirements on [env_logger](https://github.com/rust-cli/env_logger) to permit the latest version. - [Release notes](https://github.com/rust-cli/env_logger/releases) - [Changelog](https://github.com/rust-cli/env_logger/blob/main/CHANGELOG.md) - [Commits](rust-cli/env_logger@v0.10.0...v0.11.0) --- updated-dependencies: - dependency-name: env_logger dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> * Update ci.yml * Update main.rs * add #[track_caller] to the Row::get() functions This small quality-of-life improvement changes these errors: thread '<unnamed>' panicked at /../.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-postgres-0.7.10/src/row.rs:151:25: error retrieving column 0: error deserializing column 0: a Postgres value was `NULL` to: thread '<unnamed>' panicked at my-program.rs:100:25: error retrieving column 0: error deserializing column 0: a Postgres value was `NULL` * Bump CI version * Added ReadOnly session attr * Added ReadOnly session attr * Update base64 requirement from 0.21 to 0.22 Updates the requirements on [base64](https://github.com/marshallpierce/rust-base64) to permit the latest version. - [Changelog](https://github.com/marshallpierce/rust-base64/blob/master/RELEASE-NOTES.md) - [Commits](marshallpierce/rust-base64@v0.21.0...v0.22.0) --- updated-dependencies: - dependency-name: base64 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> * Shrink query_opt/query_one codegen size very slightly * use `split_once` instead of `split` to parse lsn strings [`str::split`](https://doc.rust-lang.org/std/primitive.str.html#method.split) allocates a vector and generates considerably more instructions when compiled than [`str::split_once`](https://doc.rust-lang.org/std/primitive.str.html#method.split_once). [`u64::from_str_radix(split_lo, 16)`](https://doc.rust-lang.org/std/primitive.u64.html#method.from_str_radix) will error if the `lsn_str` contains more than one `/` so this change should result in the same behavior as the current implementation despite not explicitly checking this. * Make license metadata SPDX compliant * Update heck requirement from 0.4 to 0.5 --- updated-dependencies: - dependency-name: heck dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> * Avoid extra clone in config if possible Using `impl Into<String>` instead of `&str` in a fn arg allows both `&str` and `String` as parameters - thus if the caller already has a String object that it doesn't need, it can pass it in without extra cloning. The same might be done with the password, but may require closer look. * add simple_query to GenericClient in tokio_postgres * feat(types): add 'js' feature for wasm Enables the "js" feature of postgres-protocol. * Add RowDescription to SimpleQueryMessage * Formatting updates * Clippy compliance * Formatting * Work with pools that don't support prepared statements Introduce a new `query_with_param_types` method that allows to specify Postgres type parameters. This obviated the need to use prepared statementsjust to obtain parameter types for a query. It then combines parse, bind, and execute in a single packet. Related: sfackler#1017, sfackler#1067 * Derive Clone for Row * impl Debug for Statement The lack of this common trait bound caused some unpleasantness. For example, the following didn't compile: let x = OnceLock::new(); let stmt = db.prepare(...)?; x.set(stmt).expect(...); // returns Result<(), T=Statement> where T: Debug * Fix a few nits pointed out by clippy - ...::max_value() -> ..::MAX - delete explicit import of signed integer types * Handle non-UTF8 error fields * PR Fix: Only use single clone for RowDescription * Replace the state machine to process messages with a direct match statements * Remove query_raw_with_param_types as per PR feedback * PR Fix: Clone first then move * Address review comment to rename query_with_param_types to query_typed * Fix a clippy warning * query_typed tweaks * Fix cancellation of TransactionBuilder::start * Release postgres-protocol v0.6.7 * Release postgres-types v0.2.7 * Release tokio-postgres v0.7.11 * Release postgres v0.19.8 * Add jiff support * For `query_typed`, deal with the no-data case. If a query returns no data, we receive `Message::NoData`, which signals the completion of the query. However, we treated it as a no-op, leading to processing other messages and eventual failure. This PR fixes the issue and updates the `query_typed` tests to cover this scenario. * Support AIX keepalive * Remove unecessary alias for Timestamp * Update impl for Timestamp The impl now directly computes `Timestamp` rather than going through `DateTime` and `Zoned`. * Remove impl for `Zoned` `Timestamp` already has impl and is semantically accurate for mapping to `timestamptz`, unlike `Zoned`. End users can do their own conversions from `Timestamp` to `Zoned` if desired. * Rename PG_EPOCH * Fix ToSql This sets the smallest unit to microseconds when calculating time deltas. Previously, the number of microseconds was expressed improperly because the rounding was not set. * Add jiff tests and overflow checks This adds tests in the same fashion as the existing ones for `chrono` and `time`. Overflow is now handled using fallible operations. For example, `Span:microseconds` is replaced with `Span::try_microseconds`. Postgres infinity values are workiing as expected. All tests are passing. * Remove `Zoned` from docs The implementation was previously removed as per jiff author comment. * Fix `chrono::DateTime<Utc>` tests `test_date_time_params` and `test_with_special_date_time_params` could fail when run with a non-UTC system time zone. In Postgres, if no time zone is stated in the input string, then it is assumed to be in system time. This means that in these tests, a correctly parsed `DateTime<Utc>` could be compared to an incorrectly offset time. The offset component is now included in the test strings of these tests. This component is already present in the test strings for `time::OffsetDateTime`. * Release postgres-derive v0.4.6 * Release postgres-types v0.2.8 * Release tokio-postgres v0.7.12 * Release postgres v0.19.9 * feat: add ssl_negotiation option * test: updte tests for direct tls * feat: provide built-in functions for setting ALPN * refactor: pub use sslnegotiation * refactor: apply review comments * chore: update postgres for ci * Add support for cidr 0.3 under separate feature * add `load_balance_hosts` to `Debug` impl for `Config` * Fix time 0.3 infinity panics Getting `PrimitiveDateTime` panics if the value is infinity. This commit fixes the panic. An infinity test is added, mirroring the existing one for chrono. * Fix clippy needless_lifetimes * Fix clippy extra_unused_lifetimes * Fix for Rust 2024 match ergonomics Fixes an error related to https://doc.rust-lang.org/nightly/edition-guide/rust-2024/match-ergonomics.html * Fix another needless_lifetimes * Fix clippy needless_borrowed_reference The applied suggestion for 2024 match ergonomics trips another lint. This fixes the lint. * Bump CI Rust version to 1.75.0 cargo tests in CI are [failing](https://github.com/sfackler/rust-postgres/actions/runs/12862700447/job/35858038081?pr=1198) because of a dependency requirement: ``` Run cargo test --all error: package `geo-types v0.7.15` cannot be built because it requires rustc 1.75 or newer, while the currently active rustc version is 1.74.0 Either upgrade to rustc 1.75 or newer, or use cargo update [email protected] --precise ver where `ver` is the latest version of `geo-types` supporting rustc 1.74.0 ``` This bumps the Rust version so tests will run. * Bump actions/checkout * Empty commit (re-run CI) * Update rand requirement from 0.8 to 0.9 Updates the requirements on [rand](https://github.com/rust-random/rand) to permit the latest version. - [Release notes](https://github.com/rust-random/rand/releases) - [Changelog](https://github.com/rust-random/rand/blob/master/CHANGELOG.md) - [Commits](rust-random/rand@0.8.5...0.9.0) --- updated-dependencies: - dependency-name: rand dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> * refactor: address review comments * chore: addressed rand 0.9's deprecations * fix build * fix clippy * fix wasm build * bump getrandom * Revert time 0.2 change * Propagate features * add docs * fix ci * Release postgres-native-tls v0.5.1 * Release postgres-openssl v0.5.1 * Release postgres-protocol v0.6.8 * Release postgres-types v0.2.9 * Release tokio-postgres v0.7.13 * Release postgres v0.19.10 * Fix typo in version in postgres’s changelog * Only call set_tcp_user_timeout when enabled * feat: support jiff v0.2 * WIP * WIP * WIP * WIP * WIP * WIP * WIP * Clearly indicate tademdrive changelog * Update changelog. * Fix changelog * Fix clippy * Fix clippy --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: James Guthrie <[email protected]> Co-authored-by: Steven Fackler <[email protected]> Co-authored-by: Michael P. Jung <[email protected]> Co-authored-by: Troy Benson <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Charles Samuels <[email protected]> Co-authored-by: chandr-andr (Kiselev Aleksandr) <[email protected]> Co-authored-by: novacrazy <[email protected]> Co-authored-by: laxjesse <[email protected]> Co-authored-by: Paolo Barbolini <[email protected]> Co-authored-by: Yuri Astrakhan <[email protected]> Co-authored-by: vsuryamurthy <[email protected]> Co-authored-by: Duarte Nunes <[email protected]> Co-authored-by: Dane Rigby <[email protected]> Co-authored-by: Ramnivas Laddad <[email protected]> Co-authored-by: Lev Kokotov <[email protected]> Co-authored-by: Sidney Cammeresi <[email protected]> Co-authored-by: Allan Zhang <[email protected]> Co-authored-by: Qiu Chaofan <[email protected]> Co-authored-by: Ning Sun <[email protected]> Co-authored-by: Sidney Cammeresi <[email protected]> Co-authored-by: Harris Kaufmann <[email protected]> Co-authored-by: Allan <[email protected]> Co-authored-by: Kristof Mattei <[email protected]> Co-authored-by: Charmander <[email protected]> Co-authored-by: Shawn L. <[email protected]>
I investigated the packets that the rust postgres driver sends, and when using the extended protocol, a query executed when using an extended protocol execution method first sends a packet for the parse, and then combines the bind and execution in the next packet.
This is understandable for when using a prepared statement, however when the used query is not a prepared statement, this should be possible to reduce in a singe call? I just looked at the java packets, and JDBC seems to perform this in one packet.
This can mean a significant reduction of overhead for small queries where client response time is mostly composited of network times of flight, and potentially reduces CPU.
I have investigated the query method for as extended protocol execution and extended protocol with prepared statement, and the simple_query method for simple query protocol.
The text was updated successfully, but these errors were encountered: