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

sql: support diesel-rs #13787

Open
3 of 4 tasks
tbg opened this issue Feb 24, 2017 · 9 comments
Open
3 of 4 tasks

sql: support diesel-rs #13787

tbg opened this issue Feb 24, 2017 · 9 comments
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL C-investigation Further steps needed to qualify. C-label will change. meta-issue Contains a list of several other issues. X-nostale Marks an issue/pr that should be ignored by the stale bot
Milestone

Comments

@tbg
Copy link
Member

tbg commented Feb 24, 2017

The following are known to block supporting diesel-rs.

The list is likely incomplete, but with hacky workarounds to these I was at least able to create a simple table and read from it.

cc @cuongdo

Jira issue: CRDB-6111

@tbg tbg added the A-sql-pgcompat Semantic compatibility with PostgreSQL label Feb 24, 2017
@petermattis petermattis added this to the Later milestone Feb 24, 2017
tbg added a commit to tbg/cockroachdb-diesel-example that referenced this issue Oct 8, 2017
This is almost verbatim the example from

https://github.com/diesel-rs/diesel/tree/3ae353c3aff8a7ea64ed6cb39b3a045ac86cd60e/examples/postgres

with minor adjustments:

1. after `diesel setup`, edit the created `up.sql` and `down.sql` to contain
   only a trivial statement (like `SELECT 1`). This is necessary because by
   default they contain a [random trigger] that CockroachDB can't handle.
2. `schema.rs` was manually spelled out using the `table!` macro. The tutorial
   uses `infer_schema!` for which we don't have all the [internals]. Note that
   we use `BigInt` to properly support CockroachDB's `SERIAL` type. In turn,
   `struct Post` has `id: i64` instead of `i32`.
3. Some adjustments in `show_posts` to list the ID which is otherwise impossible
   to guess.
4. Pinned the diesel dependency for no good reason (to hopefully have this go
   stale later).

There are likely more problems not discovered by this toy example. See the
[tracking issue].

[random trigger]: https://github.com/diesel-rs/diesel/blob/master/diesel_cli/src/setup_sql/postgres/initial_setup/up.sql
[internals]: cockroachdb/cockroach#8675
[tracking issue]: cockroachdb/cockroach#13787
@tbg
Copy link
Member Author

tbg commented Oct 8, 2017

I played with this again today and while I don't remember whether I had to add more workarounds originally, it doesn't seem so bad, in fact, it's actually usable:

https://github.com/tschottdorf/cockroachdb-diesel-example

I'll probably explore using the query builder portion more (not so crazy about using ORMs generally).

@rushmorem
Copy link

rushmorem commented Jan 19, 2018

Any update on this? Diesel is now at v1.1.1. As of now, it has 15,918 recent downloads while the postgres driver has 11,114. I would love to see this ORM officially supported.

@benesch
Copy link
Contributor

benesch commented Jan 20, 2018

No updates, I'm afraid. Pull requests welcome, of course!

@benesch
Copy link
Contributor

benesch commented Jan 21, 2018

But looks like we’re making progress with #21615! Thanks, @nvanbenschoten!

@jordanlewis jordanlewis added C-investigation Further steps needed to qualify. C-label will change. meta-issue Contains a list of several other issues. labels Apr 27, 2018
@jordanlewis
Copy link
Member

jordanlewis commented Oct 6, 2020

I revisited this a bit today. It looks like there are a few test suites within the diesel source code. Here's how I ran them:

  1. Clone the diesel repo
  2. Update to the latest rust nightly (rustup update, and I think you have to have switched your release to nightly first as well)
  3. Put the following text in the .env file:
# The database to use when testing against Postgres.
PG_DATABASE_URL=postgresql://root@localhost:26257/diesel_test
# The database to use when running the Postgres examples during testing.
PG_EXAMPLE_DATABASE_URL=postgresql://root@localhost:26257/diesel_example
  1. Run ./bin/test, or for each package (diesel, diesel_cli, diesel_tests, etc) run: cargo test --no-default-features --features "postgres"

Here are the failures in the diesel package:

test result: FAILED. 34 passed; 4 failed; 0 ignored; 0 measured; 0 filtered out
    connection::transaction_manager::test::transaction_depth_is_tracked_properly_on_commit_failure
    pg::connection::tests::queries_with_sql_literal_nodes_are_not_cached
    pg::types::record::tests::record_deserializes_correctly
    pg::types::record::tests::serializing_named_composite_types

Two of them seem to be about default integer size (4 in Postgres, 8 in Cockroach). The other two:

#55233 (transaction state after serializable restart is wrong) #54954 (BEGIN/ROLLBACK when inside/outside of a txn should be a warning)
#55226 (fractional years are parsed wrong in intervals).

diesel_cli

    test result: FAILED. 11 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out
    infer_schema_internals::information_schema::tests::get_foreign_keys_loads_foreign_keys
    infer_schema_internals::information_schema::tests::get_table_data_loads_column_information

The first one is caused by the fact that, in Cockroach, index names are not globally unique: they're instead scoped by table. In particular, more than one table has the index called 'primary': in fact, every table with a primary index has an index called 'primary'. Diesel writes the following query and expects it to produce the output for a single index, but in fact it produces output for all indexes called 'primary' in the database:

SELECT information_schema.key_column_usage.table_name, information_schema.key_column_usage.table_schema, information_schema.key_column_usage.column_name FROM information_schema.key_column_usage WHERE ((information_schema.key_column_usage.constraint_schema = 'test_schema') AND (information_schema.key_column_usage.constraint_name = 'primary')) LIMIT 1;

The second one is (I think) caused by the int4/int8 default int size issue.

diesel_dynamic_schema

running 7 tests
test providing_custom_schema_name ... ok
test dynamic_values::nullable_dynamic_value ... ok
test dynamic_values::dynamic_query ... FAILED
test dynamic_values::mixed_value_query ... FAILED
test querying_multiple_types ... ok
test querying_basic_schemas ... ok
test columns_used_in_where_clause ... ok

failures:

---- dynamic_values::dynamic_query stdout ----
thread 'dynamic_values::dynamic_query' panicked at 'called `Result::unwrap()` on an `Err` value: DeserializationError("Unknown type: 20")', src/libcore/result.rs:1165:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

---- dynamic_values::mixed_value_query stdout ----
thread 'dynamic_values::mixed_value_query' panicked at 'Received more than 4 bytes decoding i32. Was a BigInt expression accidentally identified as Integer?', diesel/src/type_impls/integers.rs:47:9


failures:
    dynamic_values::dynamic_query
    dynamic_values::mixed_value_query

diesel_tests

This test suite fails to start due to the lack of user-defined functions, as mentioned in the issue subject. Diesel seems to make one to keep track of updated_at.

@zicklag
Copy link

zicklag commented Nov 11, 2020

So if I want to use CockroachDB with diesel are will I be fine if I don't run into one of the gotchas, or is it essentially not really going to work at all until these are fixed?

I've got an app I'm thinking of building and I'm thinking about using CockroachDB and Diesel. It's a very simple app that will probably only have like two or three tables, mostly storing basic user information necessary to have accounts and also probably some either binary or JSON data. Is it possible that I could use Diesel with Cockroach without problems in this case?

@kernfeld-cockroach
Copy link
Contributor

Hey @zicklag, although no promises because it's unsupported, it's possible that your app will work just fine. Even if you want to query JSON from Diesel, that should work okay because I see that running cargo test --no-default-features --features serde_json json in the diesel package works with CockroachDB. I would say to give it a shot and open an issue in this repo if you see a specific problem. Good luck!! 😄

@kernfeld-cockroach
Copy link
Contributor

I took a whack at this today; here are my notes.

@rafiss and I found what we think is a bug in cockroachdb, #58069

A couple augmentations to @jordanlewis 's instructions above:

  1. I don't think that nightly Rust is required; I didn't see any problems on stable Rust 1.40.0.
  2. --no-default-features is not necessary. In fact, we can run even more tests by enabling the extras feature.

As a next step, setting up a roachtest for diesel would let us easily triage test failures. By combining this with a command to set the Cockroach default int size to 4 bytes and using something like cargo test --features postgres,extras -- -Z unstable-options --format json to let us programmatically ignore tests that are known to fail, my hope is that we could soon make claims about a whitelist of Cockroach functionality that works correctly with diesel.

Copy link

github-actions bot commented May 1, 2024

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
10 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!

@rickystewart rickystewart added the X-nostale Marks an issue/pr that should be ignored by the stale bot label May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL C-investigation Further steps needed to qualify. C-label will change. meta-issue Contains a list of several other issues. X-nostale Marks an issue/pr that should be ignored by the stale bot
Projects
None yet
Development

No branches or pull requests

9 participants