Simple abstraction lifetime problem

[VersBinarii]

Simple abstraction lifetime problem

[sfackler]

See sfackler/rust-postgres#205 for a similar issue.

[VersBinarii]

Thanks for the link to the example. That helped. The following code works:

extern crate r2d2;
extern crate r2d2_postgres;
extern crate postgres;

use std::error::Error;
use std::io;

use self::r2d2_postgres::postgres::error;
use self::r2d2_postgres::postgres::types::ToSql;
use self::r2d2_postgres::postgres::rows::Rows;
use self::r2d2_postgres::{PostgresConnectionManager, TlsMode};
use self::r2d2::{Pool, InitializationError};

pub struct DB {
    pool: Pool<PostgresConnectionManager>,
}

impl DB {
    pub fn new(conn_string: &str) -> Result<DB, InitializationError> {
        let config = r2d2::Config::default();
        let manager = PostgresConnectionManager::new(conn_string, TlsMode::None)
            .expect("Failed to establish DB connection");
        match r2d2::Pool::new(config, manager) {
            Ok(conn) => Ok(DB { pool: conn }),
            Err(err) => {
                Err(err)
            }
        }
    }

    pub fn query(&self, query: &str, params: &[&ToSql]) -> Result<Rows, error::Error> {
        match self.pool.get() {
            Ok(c) => unsafe { ::std::mem::transmute(c.query(query, params)) },
            Err(err) => {
                Err(error::Error::Io(io::Error::new(io::ErrorKind::Other, err.description())))
            }
        }
    }

    pub fn execute(&self, query: &str, params: &[&ToSql]) -> Result<u64, error::Error> {
        match self.pool.get() {
            Ok(c) => unsafe { ::std::mem::transmute(c.execute(query, params)) },
            Err(err) => {
                Err(error::Error::Io(io::Error::new(io::ErrorKind::Other, err.description())))
            }
        }
    }
}

Could you comment however if the above approach is the "best" or even good?
I read the description of mem::transmute() function and it was like reading Necronomicon (oh, wait...). Is using such a dangerous tool is necessary for something that to me should be pretty much trivial task?

Thanks for the time and the library and sorry for noobish questions!

[sfackler]

You shouldn't need to do any transmuting in execute - it's just returning a Result<u64>. The implementation for query is actually wrong - the Rows will have a dangling reference to the connection, which may be moved or even shut down and deallocated while still referenced. You'd need to have a wrapper struct like the one in that issue to hold the connection alive.

I would personally try to avoid these kinds of constructs where possible in favor of taking a connection from the pool and using that when you need to. Even ignoring the correctness issues above, you're doubling your query load on the server with this approach since you end up having one pre-check out health check for every query you make.

[VersBinarii]

Yes, the execute indeed does not need the transmute.

I admit i'm not entirely sure why Rows needs the reference to the connection. I thought that Result<Rows, error::Error> returns it rows by value. Is this something i should have been able to infer from the docs?

I will try to rewrite the module to use a global pool.

Thanks for the comments.

[sfackler]

The Rows needs a reference to its Statement which contains type information about the result columns. This has a reference to the Connection so that it can clean up its server-side state when it falls out of scope.

[sfackler]

It actually looks like that's something that I can switch on my end to avoid having that dependency.

[VersBinarii]

I'm happy then that me being dense led to library improvement ;)

[sfackler]

sfackler/rust-postgres#210