Skip to content

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

Open
fritshoogland-yugabyte opened this issue Apr 5, 2023 · 12 comments

Comments

@fritshoogland-yugabyte
Copy link

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.

@sfackler
Copy link
Owner

sfackler commented Apr 5, 2023

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.

@fritshoogland-yugabyte
Copy link
Author

Thank you for your response. I had the feeling there was some reason for it.
Can you expand on that a bit?
Why can java do this without requiring PostgreSQL to have parsed it, and be able to send out parse, bind and execute in a single go?

@sfackler
Copy link
Owner

sfackler commented Apr 5, 2023

IIRC the Java client string-interpolates all of its query parameters or something.

@fritshoogland-yugabyte
Copy link
Author

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.

@sfackler
Copy link
Owner

sfackler commented Apr 5, 2023

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.

@fritshoogland-yugabyte
Copy link
Author

Check. That is clear.

@fritshoogland-yugabyte
Copy link
Author

@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?

@sfackler
Copy link
Owner

I think that would be possible with a more sophisticated implementation of ToStatement, yeah.

@fritshoogland-yugabyte
Copy link
Author

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.

@sfackler
Copy link
Owner

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.

@sfackler
Copy link
Owner

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.

ramnivas added a commit to exograph/rust-postgres that referenced this issue Jun 4, 2024
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
@tmm1
Copy link

tmm1 commented Oct 28, 2024

Can this issue be closed since #1147 was merged?

rudolphfroger pushed a commit to tandemdrive/rust-postgres that referenced this issue Mar 4, 2025
* 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]>
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

No branches or pull requests

3 participants