jackc / pgx

PostgreSQL driver and toolkit for Go
MIT License
10.83k stars 845 forks source link

Inconsistent Parameter Handling Between QueryExecModeCacheStatement and QueryExecModeSimpleProtocol #2157

Open suleimi opened 3 weeks ago

suleimi commented 3 weeks ago

I'm not sure if this is a bug or more just a consequence of how parameter handling is implemented in the pgx library, especially when using custom types. But i've just noticed that using QueryExecModeCacheStatement vs QueryExecModeSimpleProtocol when building and executing a psql query results in two different sql queries, (and in my case one of which is wrong and causes an error):

To Reproduce

package main

import (
    "context"
    "log"

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

type SomeFancyNumber int

const (
    FancyNumber1 SomeFancyNumber = iota
    FancyNumber2
)

func (s SomeFancyNumber) String() string {
    switch s {
    case FancyNumber1:
        return "fancy_number_1"
    case FancyNumber2:
        return "fancy_number_2"
    }
    return "unknown"
}

func main() {
    simpleConn, err := pgx.Connect(context.Background(), "host=postgres port=5432 user=auser password=apassword dbname=adbname sslmode=disable default_query_exec_mode=simple_protocol")
    if err != nil {
        log.Fatal(err)
    }
    defer simpleConn.Close(context.Background())

    cacheStatementConn, err := pgx.Connect(context.Background(), "host=postgres port=5432 user=auser password=apassword dbname=adbname sslmode=disable default_query_exec_mode=cache_statement")
    if err != nil {
        log.Fatal(err)
    }
    defer cacheStatementConn.Close(context.Background())

    // Create a table and put one record in it
    //
    _, err = simpleConn.Exec(context.Background(), `CREATE TABLE IF NOT EXISTS test (id SERIAL PRIMARY KEY, anumber INT NOT NULL);`)
    if err != nil {
        log.Fatalf("Failed to create table: %v", err)
    }
    log.Println("Table 'test' created successfully!")

    _, err = simpleConn.Exec(context.Background(), `INSERT INTO test (anumber) VALUES ($1);`, FancyNumber1) // Replace 42 with the desired value
    if err != nil {
        log.Printf("Simple protocol exec resulted in error %v", err)
    } else {
        log.Println("Record inserted successfully into 'test' table using simple protocol mode!")
    }

    _, err = cacheStatementConn.Exec(context.Background(), `INSERT INTO test (anumber) VALUES ($1);`, FancyNumber1) // Replace 42 with the desired value
    if err != nil {
        log.Printf("Cache statement exec resulted in error %v", err)
    } else {
        log.Println("Record inserted successfully into 'test' table using cache statement mode!")
    }
}

Expected behavior

identical or equivalent resolved queries irregardless of the execution modes of simple_protocol and cache_statement

i.e a log entry of both

Record inserted successfully into 'test' table using simple protocol mode!
Record inserted successfully into 'test' table using cache statement mode!

Actual behavior

The queries are resolved differently for the different execution modes.

For default_query_exec_mode=simple_protocol; the query resolves to:

INSERT INTO test (anumber) VALUES ("fancy_number_1");

For default_query_exec_mode=cache_statement; the query resolves to:

INSERT INTO test (anumber) VALUES (0);

Version

This can be worked around by explicitly converting SomeFancyNumber to an int before passing it to the Exec function. While this may not a bug in the strictest sense, it does represent a design quirk in the library that may be helpful to document in the pgx library usage guidelines. Users switching modes between cache_statement and simple_protocol might not expect that some of their queries need to be tweaked/refactor.

jackc commented 3 weeks ago

This behavior is undesirable, but I believe it is ultimately unavoidable unless you can give pgx some help.

When using QueryExecModeCacheStatement, pgx knows that the desired type for $1 is a PostgreSQL int4. It tries to find a path to a binary encoded PostgreSQL int4 and finds it by converting SomeFancyNumber to its underlying Go int type.

When using QueryExecModeSimpleProtocol pgx can only use the text protocol and it doesn't know what the PostgreSQL type is. You can let pgx know the desired type with:

simpleConn.TypeMap().RegisterDefaultPgType(SomeFancyNumber(0), "int4")

Unfortunately, that is not quite enough. pgx tries to convert SomeFancyNumber to directly to a PostgreSQL int4 first, but it can't. As mentioned above, the simple protocol must use the text protocol. So pgx tries to convert SomeFancyNumber to a string which it can because SomeFancyNumber implements fmt.Stringer. pgx finds that path to encode the value before it finds the path of converting SomeFancyNumber to its underlying Go int type.

What does solve the problem is also implementing pgtype.Int64Valuer. That path is found before the fmt.Stringer path.

func (s SomeFancyNumber) Int64Value() (pgtype.Int8, error) {
    return pgtype.Int8{Int64: int64(s), Valid: true}, nil
}

I updated the documentation to clarify this in c76a650f75781d7114612d848922868ab190e868.