Scanning Nil-able Custom Types

[jaltavilla]

I'm having problems scanning into custom types, that can have null returned for them. Instead of scan invoking the object's DecodeText method, scan return an error such as can't scan into dest[1]: unknown oid 16494 cannot be scanned into &{%!t(string=) %!t(*time.Duration=<nil>) []}).

Here is a psuedocode example of what I'm trying to do.

-- Postgres record type
CREATE TYPE foo_t AS (
    field1    TEXT, 
    // more fields...
);

// Define a go type to match a postgres record type
type Foo struct { 
     Field1 string
     // more fields...
}

func (f *Foo) DecodeText(ci *pgtype.ConnInfo, buf []byte) error {
   // parse the composite record like ('a',,'b') into this object's fields
}

// Retrieve some data into this custom type
var when *time.Time
var f *Foo
row := conn.QueryRow(ctx, "SELECT NULL::timestamp, NULL::foo_t")
err := row.Scan(&when, &f)

I believe an error is returned because the type passed to ConnInfo Scan is a pointer to a pointer; which means it does not implement the BinaryEncoder or BinaryDecoder interface. The custom type's oid isn't registered with ci (and there is nothing implementing the DataType interface to register). Is there another way to accomplish what I'm trying to do?

While experimenting to try and understand the problem, I was able to make Scan work in this case, by adding a bit of reflection. I can make a pr to add this code if that's the right solution. Just prior to falling back to scanUnknownType:

    // We might be given a pointer to something that implements the decoder interface(s),
    // even though the pointer itself doesn't.
    refVal := reflect.ValueOf(dest)
    if refVal.Kind() == reflect.Ptr && refVal.Type().Elem().Kind() == reflect.Ptr {
        // If the database returned NULL, then we leave dest as nil to indicate that.
        if buf == nil {
            return nil
        }

        // We need to allocate an element, and set the destination to it
        // Then we can retry as that element.
        // This is roughly equivelent to the following go code:
        //      var dest **Foo
        //      elemPtr := new(Foo)
        //      *dest = elemPtr
        elemPtr := reflect.New(refVal.Type().Elem().Elem())
        refVal.Elem().Set(elemPtr)
        return ci.Scan(oid, formatCode, buf, elemPtr.Interface())
    }

[jackc]

I think this is the correct approach. We are already doing something similar with scanUnknownType calling GetAssignToDstType. Perhaps there is a refactoring opportunity.

What's definitely clear though is that we need a test for custom types with unregistered OIDs . I added a failing test to branch pointer-to-custom-type. Maybe that would help with implementing the fix.

[jaltavilla]

Thanks, I'll take a look at that branch.

[jaltavilla]

I spent some time trying to figure out how to refactor things so that the handling of pointer to pointers wasn't duplicated. I didn't have much luck though.

The simplest starting point I saw, was to change scanUnknownType to recursively call ConnInfo Scan on the result of GetAssignToDstType. This handled the non-null case correctly. I didn't see a clean way to change GetAssignToDstType to deal with the null case though.

Most callers of GetAssignToDstType choose between it and NullAssignTo based on the value they are going to assign. Scan can only choose between the two if dest is a pointer to a pointer. However, only GetAssignToDstType knows if it is a pointer to a pointer. I considered trying to merge the two functions, and have an is null flag, but that didn't seem as clean as it was now.

That made me think, that the real problem is that assigning null to a pointer to a pointer is special. Null values are handled elsewhere for all other types. I tried to make null less special by just starting scan with checking for nil buf, and just calling NullAssignTo in this case. That would mean that recursing into Scan for non-nulls should work. However, this change broken most tests, as NullAssignTo only supports a few types. Given that the effects of changing this would only be seen at runtime, I wasn't sure if it was safe to support more types.

I next thought, that maybe the way to make null not special for pointers to pointers would be to make a DataType to represent this. In which case it could recursively scan or handle null like other data types. However, there wouldn't be an oid to indicate it should be used. Instead, Scan would have to use reflection to detect this was a pointer to a pointer. At that point, it seemed simplest to just do the reflection in Scan.

Ultimately I made a pr into into your branch of code, that just has the special case for pointers to pointers in Scan. I figured that way you could use it as a starting point, if you see a cleaner way, or merge it to master if you don't.

[jackc]

Sounds good. I merged it in without change. I also tagged a new patch release for pgtype and for pgx.

[PaulForgey]

There's still a problem if the field type has an oid and the destination is a Scanner. *type may be a sql.Scanner, but **type is not, and so scanPlanDataTypeAssignTo will happen instead.

[jackc]

@PaulForgey I'm not sure if this related to what you are saying, but 9d7fc8e corrected a bug with *foo vs. **foo. So maybe this is resolved on master.