Skip to content
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

Basic support for ltree #1696

Merged
merged 20 commits into from
Feb 16, 2022
Merged

Basic support for ltree #1696

merged 20 commits into from
Feb 16, 2022

Conversation

cemoktra
Copy link
Contributor

@cemoktra cemoktra commented Feb 10, 2022

closes #196

@cemoktra cemoktra mentioned this pull request Feb 10, 2022
@cemoktra
Copy link
Contributor Author

cemoktra commented Feb 10, 2022

Also maybe you can help, cause querying works but inserting has problems:
thread 'main' panicked at 'not implemented'

@paolobarbolini
Copy link
Contributor

Also maybe you can help, cause querying works but inserting has problems: thread 'main' panicked at 'not implemented'

I'll try testing it locally this evening

@paolobarbolini
Copy link
Contributor

Also maybe you can help, cause querying works but inserting has problems: thread 'main' panicked at 'not implemented'

I'll try testing it locally this evening

It looks like something might not have been registered properly, as sqlx::query! prints unsupported type ltree for param.

When I run an insert with sqlx::query (not the macro) it errors out with Database(PgDatabaseError { severity: Error, code: "XX000", message: "cache lookup failed for type 16394", detail: None, hint: None, position: None, where: None, schema: None, table: None, column: None, data_type: None, constraint: None, file: Some("lsyscache.c"), line: Some(2497), routine: Some("getBaseTypeAndTypmod") })

@paolobarbolini
Copy link
Contributor

Everything fixes itself if I use 16386 as the OID instead of 16394 (found out by doing SELECT * FROM pg_type WHERE typname = 'ltree')

@cemoktra
Copy link
Contributor Author

cemoktra commented Feb 11, 2022

That is strange, if i change the OID to your value, my example does not work anymore:

unsupported type ltree for param #1

Is there a list about OIDs somewhere? I also found it like you did. Guess it should be handled as Custom type maybe

@paolobarbolini
Copy link
Contributor

PostgreSQL in general doesn't guarantee type OID stability for types created by extensions

https://stackoverflow.com/a/49990732

😞

@cemoktra
Copy link
Contributor Author

cemoktra commented Feb 11, 2022

So then it should be a PgCustomType i guess. Let me try to understand how this works

@cemoktra
Copy link
Contributor Author

Ok seems i still need to add an OID to the PgCustomType struct, that's odd

@cemoktra
Copy link
Contributor Author

The name in fetch_type_by_oid is stable

sqlx-core/src/error.rs Outdated Show resolved Hide resolved
@cemoktra
Copy link
Contributor Author

I was checking a bit on lquery but I guess this should go in separate PR later as it's a bit more complicated

@abonander
Copy link
Collaborator

abonander commented Feb 11, 2022

This is looking good now, although I'd like to see this covered in the type tests.

test_type!(ltree<sqlx::postgres::types::PgLTree>(Postgres,
    "'Foo.Bar.Baz.Quux'::ltree" == PgLTree::from_str("Foo.Bar.Baz.Quux").unwrap(),
    "'Alpha.Beta.Delta.Gamma'::ltree" == PgLTree::from_iter(["Alpha", "Beta", "Delta", "Gamma"]).unwrap()
);

test_type!(ltree_vec<Vec<sqlx::postgres::types::PgLTree>>(Postgres,
    "['Foo.Bar.Baz.Quux', 'Alpha.Beta.Delta.Gamma']::ltree[]" == 
        vec![
            PgLTree::from_str("Foo.Bar.Baz.Quux").unwrap(),
            PgLTree::from_iter(["Alpha", "Beta", "Delta", "Gamma"]).unwrap()
        ]
);

(Ideally, make sure to cover paths of different lengths too.)

Unit tests for at least the FromStr impl and from_iter() method in ltree.rs couldn't hurt either.

@cemoktra
Copy link
Contributor Author

cemoktra commented Feb 11, 2022

Will try to get this done this weekend. But first of all it's family time now

@cemoktra
Copy link
Contributor Author

cemoktra commented Feb 12, 2022

@abonander don't bother, i solved the array stuff. Now i'm just wondering what went wrong on the CI about time and MySQL. Cause i did not touch that at all. fmt should be fine now too

@abonander
Copy link
Collaborator

@cemoktra the MySQL breakage should be fixed on master, you just need to rebase.

@cemoktra
Copy link
Contributor Author

@abonander rebased

@cemoktra
Copy link
Contributor Author

What went wrong this time? @abonander

@cemoktra
Copy link
Contributor Author

@abonander i found the problem in the .sql code

));

test_type!(ltree_vec<Vec<sqlx::postgres::types::PgLTree>>(Postgres,
"array['Foo.Bar.Baz.Quux', 'Alpha.Beta.Delta.Gamma']::ltree[]" ==
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize this until I checked but Gamma actually comes before Delta in the Greek alphabet.

You don't need to change this, it's just an observation.

@cemoktra
Copy link
Contributor Author

@abonander I don't understand what's wrong now. Can you help me?

@abonander
Copy link
Collaborator

ltree only has a binary wire encoding in Postgres 13 and up: #196 (comment)

We've run into this same problem with a few other types, and we don't have a good solution right now. I think the solution is to dynamically fall back to text encoding if a given type doesn't have binary encoding functions defined, but that's out of scope for this PR.

I think for now we can just hack in a way to disable this test for the 9_6 CI runs.

I'm thinking we mark the type tests with #[cfg(postgres_14)] and set RUSTFLAGS=--cfg postgres_14 for the corresponding CI runs. I'll probably do that myself and commit it to your branch.

@cemoktra
Copy link
Contributor Author

Ok thanks

@abonander abonander merged commit 6674e8b into launchbadge:master Feb 16, 2022
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

Successfully merging this pull request may close these issues.

[Postgres] Add support for ltree
4 participants