Repository: diesel-rs/diesel (language: rust)

Issue:
Postgresql error context (where) information is missing, please consider including 'W' context error field.
### Feature Flags

postgresql

## Problem Description

Error reporting with ```diesel::result::Error::DatabaseError``` is missing context information for postgresql. This makes it harder to troubleshoot where database error happens. 

### What are you trying to accomplish?

E.g. I have a before delete trigger that calls a function that has more fields in ```INSERT INTO (fields...)``` than SELECT that follows.

Error presented by many clients when I try to delete the row that would trigger the trigger/function is:

```
SQL Error [42601]: ERROR: INSERT has more target columns than expressions
  Where: PL/pgSQL function fn_insert_point_log_on_deleted_collection() line 6 at SQL statement
```

### What is the actual output?

Diesel most detailed presentation of the error is:

```
"error":"INSERT has more target columns than expressions, kind: __Unknown, details: None, hint: None, table: None, column: None, constraint: None"
```

Only message field is non-empty in this case via Diesel:

```
INSERT has more target columns than expressions
```

kind, details, hint, table, column, constraint name are all None.

### What is the expected output?

One would expect diesel to provide that "where" bit of the information:

```
  PL/pgSQL function fn_insert_point_log_on_deleted_collection() line 6 at SQL statement
```

Per 
```rust
enum ResultField {
    SqlState = 'C' as i32,
    MessagePrimary = 'M' as i32,
    MessageDetail = 'D' as i32,
    MessageHint = 'H' as i32,
    TableName = 't' as i32,
    ColumnName = 'c' as i32,
    ConstraintName = 'n' as i32,
}
```

And trait:
```rust
pub trait DatabaseErrorInformation {
fn message(&self) -> &str;
fn details(&self) -> Option<&str>;
fn hint(&self) -> Option<&str>;
fn table_name(&self) -> Option<&str>;
fn column_name(&self) -> Option<&str>;
fn constraint_name(&self) -> Option<&str>;
}
```

There is no way to convey that context/where information that is contained in https://github.com/postgres/postgres/blob/master/src/include/postgres_ext.h:
```c
#define PG_DIAG_CONTEXT			'W'
```

It would be great if someone can decide if DatabaseErrorInformation deserves a new method like context() or if this context information can be added to the "details()" implementation of PgErrorInformation trait implementation:

```rust
impl DatabaseErrorInformation for PgErrorInformation {
    fn message(&self) -> &str {
        get_result_field(self.0.as_ptr(), ResultField::MessagePrimary)
            .unwrap_or_else(|| self.0.error_message())
    }

    fn details(&self) -> Option<&str> {
        get_result_field(self.0.as_ptr(), ResultField::MessageDetail)
    }
```

Adding this context information is something that would help the error logging in common, but I also think that it can improve situation with reporting migration errors? See #2378 - More verbose errors on diesel migrations?

I can help with implementation, but this is something that spans multiple database systems as ```pub trait DatabaseErrorInformation``` is generic and I'd need guidance on how to implement. I can imagine providing a PR with adding 'W' Context into ResultField and adding this to ```details``` of PgErrorInformation, but I'm not sure this would be the best approach.

Or I'm missing some easy way of getting this context information without extending anything?


Discussion / hints:
In https://github.com/diesel-rs/diesel/pull/1572, `wg` tried to solve a similar problem. However, it has not been completed yet.
> In #1572, `wg` tried to solve a similar problem. However, it has not completed yet.

Suggested 'P' returns a string with a decimal integer of error position in the SQL.

Actually for the kind of error that is described in this issue, it returns as None.

I commented on the issue you mentioned. Hopefully the fact that 'P' contains a string with a decimal integer value may help to move with the awaiting PR/merge.

Though I'd still believe that adding context() based on 'W' is quite beneficial for troubleshooting the errors:

> PG_DIAG_CONTEXT
> An indication of the context in which the error occurred. Presently this includes a call stack traceback of active procedural language functions and internally-generated queries. The trace is one entry per line, most recent first.

May I prepare a PR with a context() method, similar to what was done in #1572 for 'P'?
Opening a PR would certainly make it easier to discuss the design. Otherwise this seems like a meaningful addition for me.

SqlState also would be helpful as a separate getter.

Many DBMS supports error codes beyond string message 

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index de25794e15ab..b70c542cd9cb 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -54,6 +54,8 @@ for Rust libraries in [RFC #1105](https://github.com/rust-lang/rfcs/blob/master/
 
 * Added support for `UNION`, `UNION ALL`, `INTERSECT`, `INTERSECT ALL`, `EXCEPT`, `EXCEPT ALL` clauses
 
+* Added the error position for PostgreSQL errors
+
 ### Removed
 
 * All previously deprecated items have been removed.
diff --git a/diesel/src/pg/connection/mod.rs b/diesel/src/pg/connection/mod.rs
index 6606c50a3ba8..306ffb93c45f 100644
--- a/diesel/src/pg/connection/mod.rs
+++ b/diesel/src/pg/connection/mod.rs
@@ -185,8 +185,26 @@ mod tests {
     use super::*;
     use crate::dsl::sql;
     use crate::prelude::*;
+    use crate::result::Error::DatabaseError;
     use crate::sql_types::{Integer, VarChar};
 
+    #[test]
+    fn malformed_sql_query() {
+        let connection = connection();
+        let query =
+            crate::sql_query("SELECT not_existent FROM also_not_there;").execute(&connection);
+        if let Err(err) = query {
+            match err {
+                DatabaseError(_, string) => {
+                    assert_eq!(Some(26), string.statement_position());
+                }
+                _ => unreachable!(),
+            }
+        } else {
+            unreachable!();
+        }
+    }
+
     #[test]
     fn prepared_statements_are_cached() {
         let connection = connection();
diff --git a/diesel/src/pg/connection/result.rs b/diesel/src/pg/connection/result.rs
index 782e1f315967..abae6dbee250 100644
--- a/diesel/src/pg/connection/result.rs
+++ b/diesel/src/pg/connection/result.rs
@@ -167,6 +167,11 @@ impl DatabaseErrorInformation for PgErrorInformation {
     fn constraint_name(&self) -> Option<&str> {
         get_result_field(self.0.as_ptr(), ResultField::ConstraintName)
     }
+
+    fn statement_position(&self) -> Option<i32> {
+        let str_pos = get_result_field(self.0.as_ptr(), ResultField::StatementPosition)?;
+        str_pos.parse::<i32>().ok()
+    }
 }
 
 /// Represents valid options to
@@ -182,6 +187,7 @@ enum ResultField {
     TableName = 't' as i32,
     ColumnName = 'c' as i32,
     ConstraintName = 'n' as i32,
+    StatementPosition = 'P' as i32,
 }
 
 fn get_result_field<'a>(res: *mut PGresult, field: ResultField) -> Option<&'a str> {
diff --git a/diesel/src/result.rs b/diesel/src/result.rs
index 38b458f3c54d..98fe88730004 100644
--- a/diesel/src/result.rs
+++ b/diesel/src/result.rs
@@ -156,6 +156,10 @@ pub trait DatabaseErrorInformation {
     /// Currently this method will return `None` for all backends other than
     /// PostgreSQL.
     fn constraint_name(&self) -> Option<&str>;
+
+    /// An optional integer indicating an error cursor position as an index into
+    /// the original statement string.
+    fn statement_position(&self) -> Option<i32>;
 }
 
 impl fmt::Debug for dyn DatabaseErrorInformation + Send + Sync {
@@ -168,7 +172,6 @@ impl DatabaseErrorInformation for String {
     fn message(&self) -> &str {
         self
     }
-
     fn details(&self) -> Option<&str> {
         None
     }
@@ -184,6 +187,9 @@ impl DatabaseErrorInformation for String {
     fn constraint_name(&self) -> Option<&str> {
         None
     }
+    fn statement_position(&self) -> Option<i32> {
+        None
+    }
 }
 
 /// Errors which can occur during [`Connection::establish`]


Test patch: