jackc / pgx

PostgreSQL driver and toolkit for Go
MIT License
10.38k stars 821 forks source link

convertSimpleArgument converts everything to string #1551

Open br3w0r opened 1 year ago

br3w0r commented 1 year ago

Could you please explain why convertSimpleArgument in values.go converts everything to string? If I understood correctly, it then breaks the result from sanitize.SanitizeSQL in (*Conn).SanitizeForSimpleQuery. I tried to make a simple test and the result is as expected – int64 got converted to string

image image
jackc commented 1 year ago

It's been a while so I don't remember the reason for sure. It could be a mistake. It also might be that it was not considered necessary since should always work thanks to PostgreSQL doing type conversion automatically.

It's also simpler -- I took a quick stab at bypassing convertSimpleArgument for primitive types such as int64 and float64. I quickly ran into some test failures because you can't rely on a float not needing quotes. Infinity and NaN complicate things.

I'm not against someone tightening this logic up if they want. But given that this path only occurs when parameters and results are in the text format I don't think it makes any difference when executing queries.

br3w0r commented 1 year ago

Yes, it doesn't make any difference most of the time, but with UPDATE ... FROM (VALUES (...)) statement it breaks the query as we don't know the concrete types in VALUES. Given an example for my test:

func Test_test(t *testing.T) {
    conn, err := pgx.Connect(context.Background(), "user=postgres password=sample_pass host=127.0.0.1 port=57333 dbname=test-db sslmode=disable")
    assert.Nil(t, err)

    res, err := conn.SanitizeForSimpleQuery(
`
UPDATE
    test_table as t1
SET
    other_id = t2.other_id
FROM
    (
        VALUES
            (
                $1,
                $2
            )
    ) as t2(
        id,
        other_id
    )
WHERE
    t1.id = t2.id
`, int64(1), int64(123))

    fmt.Println(res)
}

With a table and data:

create table test_table(
    id int8 not null,
    other_id int8 not null
);

insert into test_table values (1, 111);

SanitizeForSimpleQuery produces a query:

UPDATE
    test_table as t1
SET
    other_id = t2.other_id
FROM
    (
        VALUES
            (
                '1',
                '123'
            )
    ) as t2(
        id,
        other_id
    )
WHERE
    t1.id = t2.id

And this query passed to a DB gives an error:

SQL Error [42883]: ERROR: operator does not exist: bigint = text
Hint: No operator matches the given name and argument type(s). You might need to add explicit type casts.
br3w0r commented 1 year ago

If you agree that it's a problem, I'd like to try fixing it this weekend

jackc commented 1 year ago

If you agree that it's a problem, I'd like to try fixing it this weekend

Please do!

Se7ge commented 8 months ago

@br3w0r Hi! Any progress with the issue?

br3w0r commented 7 months ago

@Se7ge the only fine solution I found is to revert back changes to this method and leave it the same as in v3: https://github.com/jackc/pgx/blob/a413de981978e4100d5736af7aaa2b392511612f/values.go#L28

Maybe there's a better way, but unfortunately I didn't have time to find it.