limitation in how parameters are passed to `*_raw` methods and `ToSql`

[vemoo]

I was trying to implement inserting a lot of rows in batches and I think I found a limitation in the way parameters are passed to the *_raw methods. Here's some code to explain what I was trying to do (the code is incomplete, I've added just enough to ilustrate the issue):

struct Data {
    id: u16,
    data: serde_json::Value,
}

async fn insert_data(cnn:&tokio_postgres::Client, xs:&[Data]) -> Result<()> {
    let sql = ... // sql to insert xs.len() rows
    let params = ... // create some iterator from xs
    cnn.query_raw(sql, prams).await?;
    Ok(())
}

Next I tried to implement an interator that returns all the parameters for all rows from xs. So I created the following:

struct DataParams<'a> {
    xs:&'a [Data]
}

impl<'a> Iterator for DataParams<'a> {
    type Item = &'a dyn tokio_postgres::types::ToSql;
    ...
}

But this doesn't work because u16 and serde_json::Value don't implement ToSql and I need to convert or wrap them in a type that does implement it, so i cannot return an element with lifetime 'a. Then I had the following idea:

enum DataParam<'a> {    
    Id(i32),
    Data(tokio_postgres::types::Json(&'a serde_json::Value)),
}
impl<'a> Iterator for DataParams<'a> {
    type Item = DataParam<'a>;
    ...
}
impl<'a> tokio_postgres::types::ToSql for DataParam<'a> {
    ...
}

but also ends up not working because query_raw needs an iterator that returns references to dyn tokio_postgres::types::ToSql, and I don't think that can be achieved with this strategy.

As a workaround I ended up creating a Vec<DataParam<'a>> and creating the iterator from it, but i started looking at the rust-postgres code to see if query_raw could be changed to receive an iterator to some type that implements ToSql. After some investigation I think that can be done but it requires adding:

impl ToSql for &dyn ToSql {
    fn to_sql(&self, ty: &Type, w: &mut BytesMut) -> Result<IsNull, Box<dyn Error + Sync + Send>> {
        (*self).to_sql_checked(ty, w)
    }

    fn accepts(_ty: &Type) -> bool {
        true
    }

    to_sql_checked!();
}

that feels a bit wrong.

Is this something that's worth investigating? or maybe I missed something, or the workaround of using a Vec is good enough?

[sfackler]

Implementing ToSql for DataParam should work just fine.

[vemoo]

But that's not enough because query_raw params constraint is I: IntoIterator<Item = &'a dyn ToSql>, it expects references, but I cannot create an iterator that returns references to DataParam without using an intermediate Vec.

[sfackler]

If you can't change the definition of Data, then you may need an intermediate buffer, yeah.

[vemoo]

What I'm trying to say is that I think it could work without having to use an intermediate buffer if query_raw, and other *_raw methods, are changed to:

pub async fn query_raw<'a, T: ?Sized, P, I, '_, '_>(
    &'_ self,
    statement: &'_ T,
    params: I
) -> Result<RowStream, Error> where
    T: ToStatement,
    P: ToSql,
    I: IntoIterator<Item = P>,
    I::IntoIter: ExactSizeIterator, 

and

impl ToSql for &dyn ToSql {
    fn to_sql(&self, ty: &Type, w: &mut BytesMut) -> Result<IsNull, Box<dyn Error + Sync + Send>> {
        (*self).to_sql_checked(ty, w)
    }

    fn accepts(_ty: &Type) -> bool {
        true
    }

    to_sql_checked!();
}

is added.

Should I try to make a PR?

[sfackler]

The accepts implementation in that definition is not correct.

[sfackler]

I spent a while trying to figure out a way to make query_raw behave like that, but unfortunately couldn't figure it out.

[vemoo]

The accepts implementation in that definition is not correct.

That's why I call to_sql_checked in to_sql.

[sfackler]

It still violates the contract for accepts.

[vemoo]

What's the reason to have accepts and to_sql_checked in ToSql instead of checking the type and returing WrongType from to_sql directly?

[sfackler]

None::<i32> should not be able to be converted into a TEXT column.

[vemoo]

I think I found something. I first tried to use Borrow but I couldn't make it work. But if I create a new trait:

trait BorrowToSql {
    fn borrow_to_sql(&self) -> &dyn ToSql;
}

impl BorrowToSql for &dyn ToSql {
    fn borrow_to_sql(&self) -> &dyn ToSql {
        *self
    }
}

impl<T: ToSql> BorrowToSql for T {
    fn borrow_to_sql(&self) -> &dyn ToSql {
        self
    }
}

then encode_bind can be written as

fn encode_bind<I, P>(
    bytes: &mut BytesMut,
    types: &[Type],
    params: I,
) -> Result<(), Box<dyn Error + Sync + Send>>
where
    P: BorrowToSql,
    I: IntoIterator<Item = P>,
    I::IntoIter: ExactSizeIterator,
{
    for (p, ty) in params.into_iter().zip(types) {
        p.borrow_to_sql().to_sql_checked(ty, bytes)?;
    }
    Ok(())
}

and it compiles, and I can use it in a small test case. I will try to change it and see if the rest of the code still works.

[sfackler]

Oh, nice! If that allows both &dyn ToSql and T: ToSql arguments, then it should handle both cases!

[vemoo]

I have implemented it for tokio-postgres here, it didn't require too many changes and it compiles and the tests pass.

I will finish making the changes and try it on my repo, and then I will create a PR.