Possible ergonomic improvements?

[githubaccount624]

First of all, I'd like to thank you for all your hard work on this project! My use-case (complex analytical queries) didn't really mesh too well with Diesel, and I've since migrated my entire codebase to tokio-postgres. The ergonomics of async are amazing and have made my code much cleaner and shorter.

However, there are a few things I miss from Diesel, and I was curious to get your thoughts on them. I think with a few small changes that this library could become seriously amazing!

1. Pass by reference

I notice that the array of arguments along with each argument itself needs to be passed by reference to a query. Would it not be more efficient and cleaner to pass small Copy values like integers by value?

2. Required to use all passed parameters to a query

Do you think it might be a good idea to remove the error that results from not using every parameter passed to the query? I use format! to assemble some complex queries, and sometimes this results in not having to use every parameter, depending on the circumstances. In Diesel, if a parameter is not used, then it is just ignored and the query proceeds regardless. To accommodate this, I'm currently appending things like AND $3 = $3 just to satisfy the restriction of having to use all parameters.

3. Is it possible to query without calling prepare first?

Some queries I have are so simple that it seems a bit redundant to have to prepare the SQL first before executing it. I see that there is a simple_query function, and apparently this uses a different protocol of which I am ignorant of (simple query protocol). I'm not sure of how or whether or not the semantics of this differs from the regular query function. Apparently rows are returned as strings instead? If so, it'd be nice to have a query function that just takes a SQL string and has otherwise identical semantics to the regular query.

4. Have a function for retrieving a single row?

It is often the case that you are only retrieving a single row from a query. In Diesel this functionality is supported with the get_result function. What do you think about including something like this?

5. Automatic deserialization into a struct

I've brought this up in a separate issue before, but I think this one is probably the most important. It is quite cumbersome to have to manually build a struct by calling get on each column. Thankfully I have found the serde_tokio_postgres crate, which provides this exact functionality, which in practice looks like this:

let mut rows = client.query(&statement, &[&person.person_id])
                     .map_ok(|r| from_row::<ResponseGetProfile>(r).unwrap())
                     .try_collect::<Vec<ResponseGetProfile>>().await?;

Also please note that this is a fork of an already small and relatively stale crate (it required a few changes to work with tokio_postgres as opposed to regular rust_postgres).

However, even this is a bit verbose, which I think could be lessened with official integration into the main library. Have you considered merging its functionality with this crate? It would be a huge ergonomic win.

Thanks for reading! Sorry if any of my suggestions are dumb. I'm still new to Rust.

[sfackler]

Would it not be more efficient and cleaner to pass small Copy values like integers by value?

What would the type signature of the method be in that case?

Do you think it might be a good idea to remove the error that results from not using every parameter passed to the query?

We could make that change, but I think there's a simpler solution than adding a bunch of no-op clauses - just slice the parameter list down to the right length: &params[..stmt.params().len()].

If so, it'd be nice to have a query function that just takes a SQL string and has otherwise identical semantics to the regular query.

This is possible in the synchronous interface (it just prepares the statement for you and then immediately uses it). I'd like to do the same for the async library as well, but it's blocked on an upstream bug getting fixed: rust-lang/rust#63832.

In Diesel this functionality is supported with the get_result function.

Unless I'm misreading, that appears to be designed around returning an inserted or modified row, not for arbitrary queries that return one row. Maybe it works anyway?

It might make sense to add something like that, though. I guess we'd probably want it to error if there isn't exactly one row? I don't want to get into a place of having dozens of convenience methods though.

However, even this is a bit verbose, which I think could be lessened with official integration into the main library.

I don't think there's anything fundamental ergonomically that can't be handled in a separate crate - extension traits can add new methods to Client, for example. I've tried to generally keep postgres and tokio-postgres themselves pretty small - TLS implementations, binary copy interfaces, etc are all separate crates.

[githubaccount624]

What would the type signature of the method be in that case?

Okay well keep in mind I only just finished reading the Rust book so I'm very new to all of this. Would something like making a new trait DatabaseParameter work and then implementing it for all the common types? So the parameter list type would be a Vector of DatabaseParameters. And things like Strings, string refs, ints, etc. could all deref to this type? Not sure really! I just assumed that since it was possible in Diesel that it could be done here too, but maybe they use macro magic.

We could make that change

Hooray! The problem with slicing the parameter list down is that depending on how you format the query, you may, for example, use only the 2nd, 4th, and 7th parameter, you know? So it'd be quite cumbersome to have to, in addition to formatting the query, also having to mess with the parameter vector list.

This is possible in the synchronous interface ... but it's blocked on an upstream bug

Ah okay well I'm glad it's being thought of in any case! Would be nice to get rid of some of those extra prepare statements

It might make sense to add something like that, though. I guess we'd probably want it to error if there isn't exactly one row? I don't want to get into a place of having dozens of convenience methods though.

I'd guess that it would return Option<Row>?

I definitely feel you on not having dozens on convenience methods, but this seems like a special case. Around half of my queries in my application select a single row, so it seems like a common enough use-case to accommodate. A query will either return one row, or many, and currently the API only accommodates the latter case.

Edit: I thought of an even more compelling reason. With the current API you need to make the rows that return a mutable variable for the sole reason to be able to call pop on the Vector (unless you want to have one really long expression). So not only is a Vector needlessly created (since you're only interested in the one item), but the row set needs to be temporarily mutable as well.

I don't think there's anything fundamental ergonomically that can't be handled in a separate crate - extension traits can add new methods to Client, for example. I've tried to generally keep postgres and tokio-postgres themselves pretty small - TLS implementations, binary copy interfaces, etc are all separate crates.

True enough, it can be definitely handled in another crate. I suppose I'm just worried because what I'm currently using to handle this functionality is a small fork of an already small crate, of which I am not knowledgeable enough to maintain myself. This seems like a relatively core functionality, as I imagine the vast majority of users of this crate are not manually deserializing every query into their own structs. It'd be nice to have some official support for this.

[sfackler]

Would something like making a new trait DatabaseParameter work and then implementing it for all the common types? So the parameter list type would be a Vector of DatabaseParameters.

How would that differ from the ToSql trait?

[sfackler]

The problem with slicing the parameter list down is that depending on how you format the query, you may, for example, use only the 2nd, 4th, and 7th parameter, you know?

A query that doesn't use some parameters fails to parse on the Postgres server unless you use prepare_typed to explicitly enumerate the types of all of the parameters:

#[tokio::test]
async fn foo() {
    let mut client = connect("user=postgres").await;

    let query = client.prepare("SELECT $2::INT8").await.unwrap();
    println!("{:?}", query.params());
}

thread 'foo' panicked at 'called `Result::unwrap()` on an `Err` value: Error { kind: Db, cause: Some(DbError { severity: "ERROR", parsed_severity: Some(Error), code: SqlState("42P18"), message: "could not determine data type of parameter $1", detail: None, hint: None, position: None, where_: None, schema: None, table: None, column: None, datatype: None, constraint: None, file: Some("postgres.c"), line: Some(1400), routine: Some("exec_parse_message") }) }', src/libcore/result.rs:1084:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

[githubaccount624]

How would that differ from the ToSql trait?

Oh, I guess it'd be a Vector of ToSql parameters then?

A query that doesn't use some parameters fails to parse

Sorry I think I didn't explain it properly, either that or I'm misunderstanding. Here's an example of what I'm looking for:

let statement = client.prepare("SELECT name FROM person WHERE person_id = $1;").await?;

let rows = client.query(&statement, &[&person_id, &5]).try_collect::<Vec<_>>().await?;

This currently gives the following runtime error:

panicked at 'expected 1 parameters but got 2'

Ideally it would sub person_id into the query and ignore the 5, since it is not used in the query.

This is useful for when you're dynamic query building and may only use a subset of the parameters depending on the frontend input to your server.

[sfackler]

Right, that's the bit that can be easily sliced away. I was referring to when you were asking about a query that "use[d] only the 2nd, 4th, and 7th parameter".

[githubaccount624]

Something like this:

let query = match user_input {
    "apple" => "SELECT name FROM person WHERE person_id = $1 AND avatar = $2 AND email_address = $3;",
    "banana" => "SELECT name FROM person WHERE person_id = $1 AND email_address = $3;"
};

It's nice to just have the parameter list always be the 3 possible parameters that you have to draw from, and able to modify the query to use whichever ones it needs. Does that make sense?

[sfackler]

Sure, but there is nothing I can do about the database not knowing what the type of $2 is in the second query.

[githubaccount624]

Okay now I'm confused. Why does the database need to know the type of $2 if it isn't even being passed to the database since it's not included in the query?

[sfackler]

It is still passed to the database. The client has no idea which parameters are and aren't mentioned in the query.

[githubaccount624]

Oh, is there a way to.. not send it to the database? Hmm, looks like there might be a need for a little helper function. I hate to use the "but Diesel" argument again, but IIRC this doesn't happen with their approach. Well in any case I'll try to write a function that helps with this.. shouldn't be too hard I don't think. I think I just need to seek through the query string and find all instances of $x, and then output a parameter list Vector that only contains those. Something like that.

[githubaccount624]

Wait nevermind that wouldn't work I'd need to change the query string too.. Agh, I just don't understand. Why does postgres have to error out just because you're passing it a parameter that it doesn't use? That seems so strange to me. It'd be like if Rust refused to compile because you didn't use a supplied parameter in a function. I'll have to think more on this.

[sfackler]

You'd have to ask the Postgres developers.

[sfackler]

You can now query directly without explicitly preparing a statement in tokio-postgres.