panic when scanning null enum to *sql.Scanner

[vetcher]

Updating from v1.8.1 to v1.12.0 fails our tests with panic when scanning null enum value from pg.

After some research, it seems that this commit https://github.com/jackc/pgtype/commit/824d8ad40daa6ab015603df0dba250769d6c0653 and issue https://github.com/jackc/pgx/issues/1211 introducing the problem:

=== RUN   TestEnumScan
--- FAIL: TestEnumScan (0.01s)
panic: reflect.Set: value of type **tmp.myEnum is not assignable to type *tmp.myEnum [recovered]
    panic: reflect.Set: value of type **tmp.myEnum is not assignable to type *tmp.myEnum

goroutine 19 [running]:
testing.tRunner.func1.2({0x1032a81c0, 0x14000193030})
    /Users/vetcher/go118/go1.18.1/src/testing/testing.go:1389 +0x1c8
testing.tRunner.func1()
    /Users/vetcher/go118/go1.18.1/src/testing/testing.go:1392 +0x384
panic({0x1032a81c0, 0x14000193030})
    /Users/vetcher/go118/go1.18.1/src/runtime/panic.go:838 +0x204
reflect.Value.assignTo({0x10329b880?, 0x0?, 0x1032886da?}, {0x1031fa0b1, 0xb}, 0x1032b2340, 0x0)
    /Users/vetcher/go118/go1.18.1/src/reflect/value.go:3062 +0x210
reflect.Value.Set({0x1032b2340?, 0x1400019c218?, 0x1032bb780?}, {0x10329b880?, 0x0?, 0x1031b139c?})
    /Users/vetcher/go118/go1.18.1/src/reflect/value.go:2088 +0xcc
github.com/jackc/pgtype.scanPlanSQLScanner.Scan({}, 0x10329b880?, 0x19c218?, 0x0, {0x0, 0x0, 0x0}, {0x10329b880?, 0x1400019c218})
    /Users/vetcher/go/pkg/mod/github.com/jackc/pgtype@v1.12.0/pgtype.go:621 +0x328
github.com/jackc/pgx/v4.(*connRows).Scan(0x1400022e1e0, {0x140001e9da8?, 0x1, 0x0?})
    /Users/vetcher/go/pkg/mod/github.com/jackc/pgx/v4@v4.17.2/rows.go:225 +0x38c
github.com/jackc/pgx/v4.(*connRow).Scan(0x1400022e1e0, {0x140001e9da8, 0x1, 0x1})
    /Users/vetcher/go/pkg/mod/github.com/jackc/pgx/v4@v4.17.2/rows.go:88 +0x90
github.com/vetcher/privateproject/internal/tests/tmp.TestEnumScan.func1({0x10330aa80?, 0x140001972c0?})
    /Users/vetcher/myproject/internal/tests/tmp/some_test.go:31 +0x184
github.com/jackc/pgx/v4.(*Conn).BeginTxFunc(0x103309eb0?, {0x103309eb0, 0x140001a0008}, {{0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}}, 0x140001e9f10)
    /Users/vetcher/go/pkg/mod/github.com/jackc/pgx/v4@v4.17.2/tx.go:121 +0xd0
github.com/jackc/pgx/v4.(*Conn).BeginFunc(...)
    /Users/vetcher/go/pkg/mod/github.com/jackc/pgx/v4@v4.17.2/tx.go:101
github.com/vetcher/privateproject/internal/tests/tmp.TestEnumScan(0x14000187a00)
    /Users/vetcher/privateproject/internal/tests/tmp/some_test.go:19 +0x118
testing.tRunner(0x14000187a00, 0x103305210)
    /Users/vetcher/go118/go1.18.1/src/testing/testing.go:1439 +0x110
created by testing.(*T).Run
    /Users/vetcher/go118/go1.18.1/src/testing/testing.go:1486 +0x300

And its curious why do we need line with reflect.Set of zero value, when type implements sql.Scanner and should decide scanning behavior itself. https://github.com/jackc/pgtype/blob/master/pgtype.go#L621

Minimal test that shows problem:

package tmp

import (
    "context"
    "errors"
    "testing"

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

func TestEnumScan(t *testing.T) {
    ctx := context.Background()
    conn, err := pgx.Connect(context.Background(), "postgresql://127.0.0.1:5432/")
    if err != nil {
        panic(err)
    }
    defer conn.Close(context.Background())

    err = conn.BeginFunc(ctx, func(tx pgx.Tx) error {
        _, err := tx.Exec(ctx, "CREATE TYPE testing_enum_type AS ENUM ('one', 'two');")
        if err != nil {
            return err
        }
        var v myEnum
        err = conn.QueryRow(context.Background(), "SELECT null::testing_enum_type;").Scan(&v)
        if err != nil {
            panic(err)
        }

        var vPtr *myEnum
        err = conn.QueryRow(context.Background(), "SELECT null::testing_enum_type;").Scan(&vPtr)
        if err != nil {
            panic(err)
        }
        return errors.New("rollback")
    })
    if err != nil {
        t.Fatal(err)
    }
}

type myEnum string

func (e *myEnum) Scan(v interface{}) error {
    if v == nil {
        return nil
    }
    x, ok := v.(string)
    if !ok {
        return errors.New("wtf")
    }
    *e = myEnum(x)
    return nil
}

Right now I dont have ideas how to fix that problem. Any suggestions?

[jackc]

I pushed a bug for the crash with reflection, but I don't think that calling Scan on a pointer to a sql.Scanner is going to work. It doesn't crash now, but it won't call Scan for vPtr.

It may be possible to restore the pre v1.12.0 -- assuming it did actually call it then (I didn't check) -- though all that reflection is pretty scary -- but I'm not sure it is a good idea. pgx supports handling NULL via scanning a pointer to pointer. It also supports NULL via a sql.Scanner. But not both at the same time. It seems to be something that only ever worked by accident.

And it definitely is not supported in pgx v5. The pointer to pointer NULL path takes precedence over the sql.Scanner path.

[vetcher]

fix lgfm, thank you!

[jackc]

👍