Regression in Rows.Scan: incorrect handling of sql.Scanner implementation in v5.7.2
#2204
Comments
It looks like the docs for
Do you have an example of this?
It think this matches current database/sql behavior. As far as |
|
Hello, I faced this issue after upgrading from
My use case: I use this wrapper in specific cases to customize the scan functionality for some db columns. type FieldWrapper struct {
ScanFn func(any) error
ValueFn func() (driver.Value, error)
}
func (f FieldWrapper) Scan(src any) error { return f.ScanFn(src) }
func (f FieldWrapper) Value() (driver.Value, error) { return f.ValueFn() }Example: var customType Foo
err := row.Scan(
FieldWrapper{ ScanFn: func(src any) error { return specialJSONUnmarshall(&customType) },
// ...
)In such case, for example, the element that implements (The example is simplified by a lot but the principle is that the wrapper does not need to be pointer.) I think the commit 5c9b565 (lines) left out the check that was previously in place where was: "Does the destination -as it is- implement the The result is that the Scan returns an error (in my case, after the upgrade) because it tries to json unmarshal to the wrapper directly. |
|
After carefully reading the issue and the code committed, I found that @gazerro point is valid. The functionality is introduced in that commit is: This means that what is happening now is: // lets say type (*Foo) implements sql.Scanner
var foo ****Foo = ...
_ = row.Scan(foo)It will "dereference" up to the (*Foo) level and call the Scan function of that type. So, eventually, it will operate on a different type / value than the one given in the Meanwhile, the current implementation of the For example, it will return false (while it shouldn't) in cases like this: // lets say type (*Foo) implements sql.Scanner
fw := FieldWrapper{ ... }
isSQLScanner(fw) // falseAs I understand it, the decision should be one of either keeping the new behavior but fixing the /1/ Adding these lines at the beginning of the if _, is := v.(sql.Scanner); is {
return true
}Adding a sample testcase that shows the issue existing currently: type Foo struct{}
func (i Foo) Scan(src any) error { return nil }
func (i Foo) Value() (driver.Value, error) { return nil, nil }
func TestIsSQLScanner(t *testing.T) {
f := Foo{} // implements sql.Scanner with non pointer receiver
assert.True(t, isSQLScanner(f)) // fails
pf := &f
assert.True(t, isSQLScanner(pf)) // passes
ppf := &pf
assert.True(t, isSQLScanner(ppf)) // passes
} |
|
@jackc thank you for the quick response. This is a test case that demonstrates the issue with the package main
import (
"context"
"fmt"
"log"
"reflect"
"github.com/jackc/pgx/v5/pgxpool"
)
func main() {
pool, err := pgxpool.New(context.Background(), "postgres://user:pass@localhost:5432/db")
if err != nil {
log.Fatal(err)
}
defer pool.Close()
// Create the enum type.
_, err = pool.Exec(context.Background(), `DROP TYPE IF EXISTS test_enum_type`)
if err != nil {
log.Print(err)
return
}
_, err = pool.Exec(context.Background(), `CREATE TYPE test_enum_type AS ENUM ('a', 'b')`)
if err != nil {
log.Print(err)
return
}
err = testQuery(pool, "SELECT 'a'", "a")
if err != nil {
log.Printf("test TEXT error: %s\n", err)
}
err = testQuery(pool, "SELECT 'a'::test_enum_type", "a")
if err != nil {
log.Printf("test ENUM error: %s\n", err)
}
err = testQuery(pool, "SELECT '{}'::jsonb", "{}")
if err != nil {
log.Printf("test JSONB error: %s\n", err)
}
}
// T implements the sql.Scanner interface.
type T struct {
v *any
}
func (t T) Scan(v any) error {
*t.v = v
return nil
}
// testQuery executes the query and checks if the scanned value matches
// the expected result.
func testQuery(pool *pgxpool.Pool, query string, expected any) error {
rows, err := pool.Query(context.Background(), query)
if err != nil {
return err
}
var got any
t := T{v: &got}
for rows.Next() {
if err := rows.Scan(t); err != nil {
return err
}
}
if err = rows.Err(); err != nil {
return err
}
if !reflect.DeepEqual(got, expected) {
return fmt.Errorf("expected %#v; got %#v", expected, got)
}
return nil
}With commit 2ec9004 (the parent of commit 5c9b565), the test passes without errors. However, with commit 5c9b565, the tests for Note that the first test on the |
|
@gazerro, regarding the hanging behaviour you are facing, these are two separate things, I think. Let me get into details: HangingThe reason that it hangs is that we have a panic from then, in your Lines 384 to 389 in bcf3fbd where the puddle pool close says: https://github.com/jackc/puddle/blob/bd09d14bd4018b6d65a9d7770e2f3ddf8b00af1c/pool.go#L177-L179
So essentially, it blocks forever. It can be fixed like this by changing func testQuery(pool *pgxpool.Pool, query string, expected any) error {
rows, err := pool.Query(context.Background(), query)
if err != nil {
return err
}
defer rows.Close() // <- this is neededThat way, it will not hang, and the panic that occurs will be revealed. PanicAfter the mentioned change on the commit 5c9b565 and due to the bug/missing check of So what eventually will happen is that it will try to directly But why the panic?
Lines 215 to 219 in bcf3fbd A testcase like this can reveal the panic: func TestJSONCodecScanToNonPointerValues(t *testing.T) {
defaultConnTestRunner.RunTest(context.Background(), t, func(ctx context.Context, t testing.TB, conn *pgx.Conn) {
n := 44
err := conn.QueryRow(ctx, "select '42'::jsonb").Scan(n)
require.Error(t, err) // I would expect an error like json.Unmarshal does.
})
}For example, in simillar case, @jackc I think there are two different issues here
PS What happens in |
|
Just merged #2213 which should resolve the issue. |
The
Rows.Scanmethod of thepgxpackage is documented as follows:According to the documentation, it accepts as arguments:
Scannerinterface,nil.Starting from version v5.7.2, point (2) is no longer true because:
sql.Scannerinterface.sql.Scannerbut point to types that do.Both changes contradict the method's documentation. In particular:
These issues were introduced in commit 5c9b565. The issue #2146, which this commit intended to resolve, did not need fixing in the first place because the original behavior was correct. The example code in the issue is incorrect. Specifically:
The second argument to
rows.Scanhas type**T, but**Tdoes not implementsql.Scanner. Therefore, theScanmethod has noScanimplementation to invoke.The example code should be corrected as follows:
Here,
r.Datamust not be passed as a pointer.As a result, commit 5c9b565 should be reverted to restore the correct behavior, which was consistent with both the documentation and expected usage.
The text was updated successfully, but these errors were encountered: