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

Example and step-by-step explanation added for composite-types in PostgreSQL #3855

Merged
merged 7 commits into from
Nov 27, 2023

Conversation

peter-scholtens
Copy link
Contributor

Two examples explain how PostgreSQL row data can correctly be interpreted as Rust data. This included an anonymous method with Record as also a safer method with FromSqlRow.

@weiznich weiznich requested a review from a team November 17, 2023 19:20
@Ten0
Copy link
Member

Ten0 commented Nov 18, 2023

I feel like for more experienced readers this would benefit from a:

TLDR; when

CREATE TYPE gray_type AS (intensity FLOAT4, suggestion TEXT);

then

Record<(f32, String)>

or

#[derive(Debug, FromSqlRow)]
pub struct GrayType {
    pub intensity: f32,
    pub suggestion: String,
}

impl FromSql<PgGrayType, Pg> for GrayType {
    fn from_sql(bytes: PgValue) -> deserialize::Result<Self> {
        let (intensity, suggestion) = FromSql::<Record<(Float, Text)>, Pg>::from_sql(bytes)?;
        Ok(GrayType {
            intensity,
            suggestion,
        })
    }
}

@peter-scholtens
Copy link
Contributor Author

I feel like for more experienced readers this would benefit...

I immediately agree that I'm a novice: I wrote this manual here to explain what happened, so for an expert it is certainly verbose. But frequently people struggle with understanding it, given the logs on the Diesel matrix channel.

Possibly this description could integrate better with the current documentation, but I've no idea where to start.

@Ten0
Copy link
Member

Ten0 commented Nov 20, 2023

Possibly this description could integrate better with the current documentation, but I've no idea where to start.

Oh I just mean that I think it would be nice to have a TL;DR section at the top of the guide, not suggesting any other particular change.

@peter-scholtens
Copy link
Contributor Author

I tried to include your suggestion, but while doing so, I see you wrote:

Record<(f32, String)>

while you actually meant this, I assume:

Record<(Float, Text)>

This is exactly the reason why I wrote to the manual: to be sure that I (and the reader) understand which types should be used where or at least have a working example to play with. For an experienced eye this may be an obvious error, but as a novice you end up in trial and error any combination of types, until you give up :-(

The sql_query commands on lines 334 and 372 can be executes in parallel during a test and create a failure. With locking the users table this is avoided.
@Ten0
Copy link
Member

Ten0 commented Nov 22, 2023

Ah I thought I had answered this but apparently I didn't submit, sorry.

while you actually meant this, I assume

Yes absolutely.

This is exactly the reason why I wrote to the manual: to be sure that I (and the reader) understand which types should be used where or at least have a working example to play with. For an experienced eye this may be an obvious error, but as a novice you end up in trial and error any combination of types, until you give up :-(

I agree it's really nice to have a manual. It's just that while reading it I felt like it was clear but rather long to read if you already know most of what you want to know, so I felt that it would benefit from a "summary" section (a.k.a. TL;DR) that would just give a quick example of just what types you need.

@@ -369,7 +370,8 @@ fn upsert_with_sql_literal_for_target_with_condition() {
let connection = &mut connection();
// This index needs to happen before the insert or we'll get a deadlock
// with any transactions that are trying to get the row lock from insert
diesel::sql_query("CREATE UNIQUE INDEX ON users (name) WHERE name != 'Tess'")
// As tests can run concurrently, lock the table of interest.
diesel::sql_query("BEGIN; LOCK TABLE users; CREATE UNIQUE INDEX ON users (name) WHERE name != 'Tess'; COMMIT;")
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK creating an index on a table shouldn't cause issues for parallel queries?
(Or if it does it should keep causing issues after creation)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've seen at least twice the following error in the CI checks (at sometimes it is not present):

---- update::upsert_with_sql_literal_for_target_with_condition stdout ----
thread 'update::upsert_with_sql_literal_for_target_with_condition' panicked at diesel_tests/tests/update.rs:374:10:
called `Result::unwrap()` on an `Err` value: DatabaseError(Unknown, "deadlock detected")

My assumption was that this is caused by the running the tests in parallel, as line 334 and 373 can both block the table. As I failed to get the tests running locally, I hoped my quick and dirty solution would solve the problem, apparently not :-(

Remove multiple statements
@weiznich weiznich enabled auto-merge November 27, 2023 16:51
@weiznich weiznich added this pull request to the merge queue Nov 27, 2023
Merged via the queue into diesel-rs:master with commit bc263af Nov 27, 2023
37 checks passed
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.

3 participants