sfackler / rust-postgres

Native PostgreSQL driver for the Rust programming language
Apache License 2.0
3.43k stars 436 forks source link

cannot convert &str to Postgres reg* types #1041

Open dmfay opened 1 year ago

dmfay commented 1 year ago

A query select $1::regclass (textual representation of a table oid) passed an &str tablename yields the error:

error serializing parameter 0: cannot convert between the Rust type `&str` and the Postgres type `regclass`

It's easy to work around (select $1::text::regclass) so I'm raising this issue to document that, but it'd be nice to get the conversions set up for regclass, regtype, regproc, regnamespace, etc!

sfackler commented 1 year ago

Those types are aliases of oid, so they would be converted to and from u32, not str. Seems reasonable to add support though.

trungda commented 1 year ago

@sfackler could you help advise how to add support for these reg types? I would be happy to contribute here.

These types are weird. They can be casted to both text and oid. I think in the syntax "select $1::regclass", "$1" should be a string?

sfackler commented 1 year ago

You'd just need to change these two lines to support the other appropriate types: https://github.com/sfackler/rust-postgres/blob/master/postgres-types/src/lib.rs#L729 https://github.com/sfackler/rust-postgres/blob/master/postgres-types/src/lib.rs#L1158

trungda commented 1 year ago

You'd just need to change these two lines to support the other appropriate types: https://github.com/sfackler/rust-postgres/blob/master/postgres-types/src/lib.rs#L729 https://github.com/sfackler/rust-postgres/blob/master/postgres-types/src/lib.rs#L1158

I looked a bit into this by writing a test:

#[tokio::test]
async fn regclass() {
    let conn = connect("user=postgres").await;
    // Create a table foo
    conn.execute("CREATE TABLE IF NOT EXISTS foo (c1 INT);", &[])
        .await
        .unwrap();
    test_type("regclass", &[(Some("foo".to_owned()), "'foo'")]).await;
}

I might be missing something but I think we should add support to cast from &str to REGCLASS (not u32)?

Thank you!

sfackler commented 1 year ago

How would you propose to do that?

trungda commented 1 year ago

I am thinking that we would have to modify FromSql and ToSql implementations of &str to accept these new types. E.g.,:

impl<'a> FromSql<'a> for &'a str {
    fn from_sql(ty: &Type, raw: &'a [u8]) -> Result<&'a str, Box<dyn Error + Sync + Send>> {
        match *ty {
            ref ty if ty.name() == "ltree" => types::ltree_from_sql(raw),
            ref ty if ty.name() == "lquery" => types::lquery_from_sql(raw),
            ref ty if ty.name() == "ltxtquery" => types::ltxtquery_from_sql(raw),
            ref ty if ty.name() == "regclass" => types::regclass_from_sql(raw), // something like this.
            _ => {
                types::text_from_sql(raw)
            },
        }
    }

and the relevant bits. What do you think?

sfackler commented 1 year ago

The wire format for regclass is a 32 bit unsigned integer.

trungda commented 1 year ago

Ah right! Let me look into how libpq handles the reg-type family.

trungda commented 1 year ago

So I think that libpq doesn't do anything magical (I was confused with psql). If we pass SELECT $1::regclass where $1 is a string, libpq would bind $1 to a string; and if we pass SELECT $1::regclass where $1 is a number, libpq would bind a number value to the $1 placeholder. In both cases, the response would be an oid. Only in psql, the tool would do an extra query to look up system catalog to convert from the oid to the table name (this is where I was confused as I thought that libpq would somehow convert the oid to the table name magically).

Now, back to our quest, I think we should implement FromSQL to serialize the u32 response from server to regclass. As for ToSql, what can we do to support SELECT $1::regclass where $1 can be either a string or a number? Or should we straight-up not support that syntax?

sfackler commented 1 year ago

The existing implementations work fairly directly with the postgres binary format of the equivalent type.

trungda commented 1 year ago

Great! So I guess as you suggested, we can just add

simple_from!(u32, oid_from_sql, REGCLASS);
simple_to!(u32, oid_to_sql, REGCLASS);

for regclass and the reg-types?

sfackler commented 1 year ago

There can only be one impl per type, so you'd need to modify the existing lines.