twpayne / go-geom

Package geom implements efficient geometry types for geospatial applications.
BSD 2-Clause "Simplified" License
839 stars 104 forks source link

Insert query does not work on pgx driver #231

Closed mylaluna closed 10 months ago

mylaluna commented 1 year ago

There is an existing closed issue that mentioned this problem before: https://github.com/twpayne/go-geom/issues/211

In my case, I got the same error for insert queries: QueryRow failed: ERROR: parse error - invalid geometry (SQLSTATE XX000)

I tested that the same query that works with lib/pq driver does not work with pgx drivers (pgx connetion and pgx pool). Considering that lib/pq is in the maintenance mode and pgx is growing popular, I think it is worth to fix it.

func main() {
    dbpool, err := pgxpool.New(context.Background(), dbSource)
    if err != nil {
        fmt.Printf("Unable to connect to database via pgx: %v\n", err)
        os.Exit(1)
    }
    defer dbpool.Close()

    c := City{
        Name:     "London",
        Location: ewkb.Point{Point: geom.NewPoint(geom.XYZ).MustSetCoords(geom.Coord{0.1275,51.50722,0.0}).SetSRID(4326)},
    }

    result, err := dbpool.Exec(context.Background(), `INSERT INTO cities (name, location) VALUES (?, ?);`, c.Name, &c.Location)

    if err != nil {
        fmt.Fprintf(os.Stderr, "QueryRow failed: %v\n", err)
        os.Exit(1)
    }
    rowsAffected:= result.RowsAffected()
    fmt.Printf("%d rows affected", rowsAffected)
}
twpayne commented 1 year ago

This library is still actively used, and should work with pgx without any changes.

Do you have binary_parameters=yes in your connection string?

Please can you post a full example that reproduces the problem. For example, you can modify the PostGIS example to use pgx instead of pq.

mylaluna commented 1 year ago

I edited the post. The binary_parameters=yes works for lib/pq but does not work for pgx. I got following error: server error (FATAL: unrecognized configuration parameter "binary_parameters" (SQLSTATE 42704))

mylaluna commented 1 year ago

As you suggested, I used your example code and created geomtest database with data populated.

I tested all the functions and they work fine with lib/pq as expected.

I then replaced lib/pq with pgx.

I commented out the populate function part as it is broken. I have already run the populate function with lib/pq before so I don't think I need it anyway.

The write function works fine with pgx. The focus is the read function which has the target insert query.

As a result of running the read command, the read function got stuck and never returned. I am not very sure if it is the same problem I got before but I can see both of them happened to the insert query.

Let me know if you need anything else.

Following is my modification:

package main

import (
    "context"
    "encoding/json"
    "errors"
    "flag"
    "fmt"
    "io"
    "os"

    "github.com/jackc/pgx/v5/pgxpool"

    geom "github.com/twpayne/go-geom"
    "github.com/twpayne/go-geom/encoding/ewkb"
    "github.com/twpayne/go-geom/encoding/geojson"
)

var (
    dsn = flag.String("dsn", "postgres://root:secret@localhost:5432/geomtest?sslmode=disable", "data source name")

    create = flag.Bool("create", false, "create database schema")
    // populate = flag.Bool("populate", false, "populate waypoints")
    read  = flag.Bool("read", false, "import waypoint from stdin in GeoJSON format")
    write = flag.Bool("write", false, "write waypoints to stdout in GeoJSON format")
)

// A Waypoint is a location with an identifier and a name.
type Waypoint struct {
    ID       int             `json:"id"`
    Name     string          `json:"name"`
    Geometry json.RawMessage `json:"geometry"`
}

// createDB demonstrates create a PostgreSQL/PostGIS database with a table with
// a geometry column.
func createDB(db *pgxpool.Pool) error {
    _, err := db.Exec(context.Background(), `
        CREATE EXTENSION IF NOT EXISTS postgis;
        CREATE TABLE IF NOT EXISTS waypoints (
            id SERIAL PRIMARY KEY,
            name TEXT NOT NULL,
            geom geometry(POINT, 4326) NOT NULL
        );
    `)
    return err
}

// populateDB demonstrates populating a PostgreSQL/PostGIS database using
// pq.CopyIn for fast imports.
// func populateDB(db *pgxpool.Pool) error {
//  tx, err := db.Begin(context.Background())
//  if err != nil {
//      return err
//  }
//  defer tx.Rollback(context.Background())
//  stmt, err := tx.Prepare(context.Background(), pq.CopyIn("waypoints", "name", "geom"), )
//  if err != nil {
//      return err
//  }
//  for _, waypoint := range []struct {
//      name string
//      geom *geom.Point
//  }{
//      {"London", geom.NewPoint(geom.XY).MustSetCoords([]float64{0.1275, 51.50722}).SetSRID(4326)},
//  } {
//      ewkbhexGeom, err := ewkbhex.Encode(waypoint.geom, ewkbhex.NDR)
//      if err != nil {
//          return err
//      }
//      if _, err := stmt.Exec(waypoint.name, ewkbhexGeom); err != nil {
//          return err
//      }
//  }
//  if _, err := stmt.Exec(); err != nil {
//      return err
//  }
//  return tx.Commit()
// }

// readGeoJSON demonstrates reading data in GeoJSON format and inserting it
// into a database in EWKB format.
func readGeoJSON(db *pgxpool.Pool, r io.Reader) error {
    var waypoint Waypoint
    if err := json.NewDecoder(r).Decode(&waypoint); err != nil {
        return err
    }
    var geometry geom.T
    if err := geojson.Unmarshal(waypoint.Geometry, &geometry); err != nil {
        return err
    }
    point, ok := geometry.(*geom.Point)
    if !ok {
        return errors.New("geometry is not a point")
    }
    _, err := db.Exec(context.Background(), `
        INSERT INTO waypoints(name, geom) VALUES ($1, $2);
    `, waypoint.Name, &ewkb.Point{Point: point.SetSRID(4326)})
    return err
}

// writeGeoJSON demonstrates reading data from a database in EWKB format and
// writing it as GeoJSON.
func writeGeoJSON(db *pgxpool.Pool, w io.Writer) error {
    rows, err := db.Query(context.Background(), `
        SELECT id, name, ST_AsEWKB(geom) FROM waypoints ORDER BY id ASC;
    `)
    if err != nil {
        return err
    }
    defer rows.Close()
    for rows.Next() {
        var id int
        var name string
        var ewkbPoint ewkb.Point
        if err := rows.Scan(&id, &name, &ewkbPoint); err != nil {
            return err
        }
        geometry, err := geojson.Marshal(ewkbPoint.Point)
        if err != nil {
            return err
        }
        if err := json.NewEncoder(w).Encode(&Waypoint{
            ID:       id,
            Name:     name,
            Geometry: geometry,
        }); err != nil {
            return err
        }
    }
    return nil
}

func run() error {
    flag.Parse()
    //db, err := sql.Open("postgres", *dsn)
    db, err := pgxpool.New(context.Background(), *dsn)

    if err != nil {
        return err
    }
    defer db.Close()
    if err := db.Ping(context.Background()); err != nil {
        return err
    }
    if *create {
        if err := createDB(db); err != nil {
            return err
        }
    }
    // if *populate {
    //  if err := populateDB(db); err != nil {
    //      return err
    //  }
    // }
    if *read {
        if err := readGeoJSON(db, os.Stdin); err != nil {
            return err
        }
    }
    if *write {
        if err := writeGeoJSON(db, os.Stdout); err != nil {
            return err
        }
    }
    return nil
}

func main() {
    if err := run(); err != nil {
        fmt.Println(err)
        os.Exit(1)
    }
}
twpayne commented 1 year ago

As a result of running the read command, the read function got stuck and never returned.

This is clearly a different problem to the original problem you reported. Can you find a way to reproduce the original problem you reported?

mylaluna commented 1 year ago

Sure. I used the waypoints table from the example and modified the geom field to geometry(PointZ,4326) instead of geometry(Point, 4326).

Here is the full setup:

import (
    "context"
    "database/sql"
    "fmt"
    "math/rand"
    "os"
    "time"

    "github.com/jackc/pgx/v5"
    "github.com/jackc/pgx/v5/pgtype"
    "github.com/jackc/pgx/v5/pgxpool"
    _ "github.com/lib/pq"
    "github.com/twpayne/go-geom"
    "github.com/twpayne/go-geom/encoding/ewkb"
)

var dbSource = "postgresql://root:secret@localhost:5432/geomtest?sslmode=disable"

const createWaypoint = `
INSERT INTO waypoints(name, geom) VALUES ($1, $2);
`

type createWaypointParams struct {
    Name string     `json:"name"`
    Geom ewkb.Point `json:"geom"`
}

func main() {
    conn := connectToPgxPool()
    defer conn.Close()
    w := createWaypointParams{
        Name: "test",
        Geom: ewkb.Point{Point: geom.NewPoint(geom.XYZ).MustSetCoords(geom.Coord{0.1275, 51.50722, 0.0}).SetSRID(4326)},
    }

    result, err := conn.Exec(context.Background(), createWaypoint, w.Name, &w.Geom)

    if err != nil {
        fmt.Fprintf(os.Stderr, "QueryRow failed: %v\n", err)
        os.Exit(1)
    }

    rowsAffected := result.RowsAffected()
    fmt.Printf("%d rows affected", rowsAffected)
}

func connectToPgxPool() *pgxpool.Pool {
    dbpool, err := pgxpool.New(context.Background(), dbSource)
    if err != nil {
        fmt.Printf("Unable to connect to database via pgx: %v\n", err)
        os.Exit(1)
    }
    return dbpool
}
twpayne commented 1 year ago

Thanks, but you shouldn't need either github.com/lib/pq or github.com/jackc/pgx/v5/pgxpool in the example.

twpayne commented 1 year ago

Note also that, depending on your configuration, pgx might not support the database/sql.Scanner and database/sql/driver.Value interfaces. You might need to wrap the geometry type and define new methods.

Here is an example from a project that uses twpayne/go-geos (note GEOS not geom):

package main

import (
        "encoding/hex"

        "github.com/jackc/pgtype"
        "github.com/twpayne/go-geos"
)

type Geom struct {
        *geos.Geom
}

func NewGeom(geom *geos.Geom) Geom {
        return Geom{
                Geom: geom,
        }
}

func (g *Geom) DecodeBinary(ci *pgtype.ConnInfo, buf []byte) error {
        if len(buf) == 0 {
                g.Geom = nil
                return nil
        }
        geom, err := geos.NewGeomFromWKB(buf)
        if err != nil {
                return err
        }
        g.Geom = geom
        return nil
}

func (g *Geom) DecodeText(ci *pgtype.ConnInfo, buf []byte) error {
        wkb, err := hex.DecodeString(string(buf))
        if err != nil {
                return err
        }
        geom, err := geos.NewGeomFromWKB(wkb)
        if err != nil {
                return err
        }
        g.Geom = geom
        return nil
}

func (g Geom) EncodeBinary(ci *pgtype.ConnInfo, buf []byte) ([]byte, error) {
        if g.Geom == nil {
                return nil, nil
        }
        wkb := g.Geom.ToWKB()
        return append(buf, wkb...), nil
}

func (g Geom) String() string {
        return g.ToWKT()
}
mylaluna commented 1 year ago

I noticed that the given code above uses "github.com/jackc/pgtype" instead of "github.com/jackc/pgx/v5/pgtype".

According to the pgx documentation, "github.com/jackc/pgtype" is used with pgx v4. In pgx v5 it is part of the https://github.com/jackc/pgx repository. There are quite a lot of updates in v5 so the code may not be valid anymore.

twpayne commented 1 year ago

I noticed that the given code above uses "github.com/jackc/pgtype" instead of "github.com/jackc/pgx/v5/pgtype".

This is correct. The code is from a project that uses jackc/pgx/v4.

teddyhartanto commented 5 months ago

Hello, I know this thread was closed almost a year ago. I just want to put a comment out here in case somebody ran into the same issue that @mylaluna and myself did.

I will paste a few snippets of ewkb.Point for context:

// Scan scans from a []byte.
func (p *Point) Scan(src interface{}) error {
    if src == nil {
        p.Point = nil
        return nil
    }
    b, ok := src.([]byte)
    if !ok {
        return ErrExpectedByteSlice{Value: src}
    }
    got, err := Unmarshal(b)
    if err != nil {
        return err
    }
    p1, ok := got.(*geom.Point)
    if !ok {
        return wkbcommon.ErrUnexpectedType{Got: p1, Want: p}
    }
    p.Point = p1
    return nil
}

// Valid returns true if p has a value.
func (p *Point) Valid() bool {
    return p != nil && p.Point != nil
}

// Value returns the EWKB encoding of p.
func (p *Point) Value() (driver.Value, error) {
    if p.Point == nil {
        return nil, nil
    }
    return value(p.Point)
}

// truncated code...

func value(g geom.T) (driver.Value, error) {
    sb := &strings.Builder{}
    if err := Write(sb, NDR, g); err != nil {
        return nil, err
    }
    return []byte(sb.String()), nil
}

I saw the same error when trying to insert a row that has a PostGIS' Geometry column:

2024/03/13 17:21:12 dbmodels: unable to insert into addresses: ERROR: parse error - invalid geometry (SQLSTATE XX000)

(^The dbmodels: unable to insert into addresses message is a log from my ORM. You can ignore that part.)

I turned on verbose logging on my Postgres instance so that I get to see from Postgres' PoV. Here's what it says:

2024-03-13 10:21:12.877 UTC [22567] ERROR:  parse error - invalid geometry
2024-03-13 10:21:12.877 UTC [22567] HINT:  "\x" <-- parse error at position 2 within geometry
2024-03-13 10:21:12.877 UTC [22567] CONTEXT:  unnamed portal parameter $8 = '...'

It's still a little vague, but at least we've got a hint: HINT: "\x" <-- parse error at position 2 within geometry

After extensive debugging, here's what I found:

  1. pgx can decode a custom Postgres type to a custom Golang type, and encode a custom Golang type to a custom Postgres type if the Scan() (of sql.Scanner interface) and Value() (of driver.Valuer interface) methods are implemented. If you're lost at this stage, I suggest taking a look at database/sql
  2. pgx internally has a map that it uses to map Postgres types (identified by an OID) to a Codec (the entity responsible to bidirectionally convert Postgres<->Go values). Check out the code here or the doc here
  3. PostGIS' Custom Type has an OID, say N, that is not registered in pgx's map. So, the Codec that pgx chooses is the encodePlanDriverValuer since our custom type implements the driver.Valuer interface
  4. Because pgx cannot find the OID N in its map, it can't determine the appropriate FormatType to use and it falls back to the default TextFormatCode. Code is here

    // FormatCodeForOID returns the preferred format code for type oid. If the type is not registered it returns the text
    // format code.
    func (m *Map) FormatCodeForOID(oid uint32) int16 {
    if fc, ok := m.oidToFormatCode[oid]; ok {
        return fc
    }
    
    if fc, ok := defaultMap.oidToFormatCode[oid]; ok {
        return fc
    }
    
    return TextFormatCode
    }
  5. pgx then eventually chooses (from the initial encodePlanDriverValuer Codec) encodePlanBytesCodecTextBytes Codec (a []byte Codec but TextFormatCode instead of BinaryFormatCode) which lead to the Encode() logic that prepends an \x into the resulting byte slice that is sent to Postgres. Code is here. That choice makes sense because if you take a look at the *ewkb.Point.Value() method, the concrete type it returns is a []byte.

    func (encodePlanBytesCodecTextBytes) Encode(value any, buf []byte) (newBuf []byte, err error) {
    b := value.([]byte)
    if b == nil {
        return nil, nil
    }
    
    buf = append(buf, `\x`...)
    buf = append(buf, hex.EncodeToString(b)...)
    return buf, nil
    }
  6. In essence, the overall effect is that pgx didn't send a Binary format to Postgres, but rather it sends a Text format with "\x" prepended, rendering PostGIS to complain: ERROR: parse error - invalid geometry (SQLSTATE XX000)

So, what's the solution here? The solution is just to register the unrecognised OID N with pgtype, as laid out in the doc. I have yet to implement a solution because I want to document my troubleshooting breakthrough here. But, I suppose you could do either of the following:

  1. Register the OID with a ByteaCodec which would force the FormatCode to be BinaryFormatCode, and that would lead to an Encode() logic that doesn't append "\x" unnecessarily.
  2. Register the OID with a custom Codec that you write yourself

In a nutshell, this issue isn't a go-geom issue. It's an issue of handling custom type in pgx.

Cheers!

twpayne commented 5 months ago

Thanks for all this!

For info, I'm now using pgx for some projects and have written a pgx to go-geos (go-geos, not go-geom) here: https://github.com/twpayne/pgx-geos

twpayne commented 5 months ago

For info, the initial go-geom for pgx is in this repo: https://github.com/twpayne/pgx-geom.

teddyhartanto commented 5 months ago

Fresh from the oven! Nice! Thank you so much for that! :P I'm going to try to implement the Codec myself for practice and it's a relief to know that I can take a peek at your implementation if mine's not working