jackc / pgtype

MIT License
314 stars 112 forks source link

Make CompositeType Status public #100

Closed jschaf closed 3 years ago

jschaf commented 3 years ago

The Status for other (all?) data types is public. Keeping status private makes it difficult to create a CompositeType with an initial value. The work around is to call Set() and create an []interface{} of the fields.

jackc commented 3 years ago

The private status field is intentional. CompositeType is not intended to use directly. It is designed to enable Query and Scan to encode arguments and decode results from and to other types. See https://pkg.go.dev/github.com/jackc/pgtype?utm_source=godoc#example-package-Composite for intended usage.

I suppose this could be revisited, but its interface is not really conducive to direct use. How does a public status field change things? All the other fields are still private.

jschaf commented 3 years ago

The main reason I ask is because creating and initializing a nested composite type is pretty tedious. The use-case is to encode query parameters. Right now, the only way to create an initialized composite type is by calling CompositeType.Set. The problem is that Set only supports []interface{}. A consequence of this is that the top-level composite type needs to know how convert all descendant composite types into []interface{}.

The alternative is to initialize a composite type with ValueTranscoders that are already Set, e.g. pgtype.Text{"foo", pgtype.Present}. This means you could compose composite types and let the children figure out how to Set themselves. The only barrier is that the status is undefined and only Set modifies it.

As an example, see https://github.com/jschaf/pggen/tree/main/example/complex_params. In order to initialize composite types for query parameters I ended up doing is creating 3 functions. See dimensions at https://github.com/jschaf/pggen/tree/main/example/complex_params.

I think the basic problem is that it's very difficult to compose composite and array types and build them up programmatically.

jackc commented 3 years ago

Sorry, its been a while since I initially wrote the composite type support and the project where I was using it ended going a different direction so my memory might be a bit fuzzy. But I do remember for sure that CompositeType was not designed to be used directly as a value container. I might be wrong on the reason, but I think it was mostly just that it is just too awkward to use directly. I think even with this change it would be rather tedious.

I took a look at the code you linked to but I still don't quite understand. Are you creating a CompositeType for each query?

FWIW, when I was using it I would create Go structs that represented by composite types and use utilities like pgtype.CompositeFields, pgtype.CompositeBinaryBuilder, or pgtype.CompositeBinaryScanner to implement EncodeBinary or DecodeBinary. This would probably perform better than constructing a graph of CompositeTypes for each query. Since you're doing code generation it might even be simple for people using your tool.


I'm not totally against making status public, but I suspect that there is another way that would be even easier to use. Here's a few ideas that may be applicable. I'd like to consider these before making this change.


Maybe CompositeType.Set should be smarter. It looks like the whole problem might be due to a deficiency there.

Take a look at https://github.com/jackc/pgtype/tree/master/pgxtype if you haven't already. It has a bunch of utilities for inspecting the database and registering types that may be useful.

Maybe there needs to be a new type in pgtype that is designed for containing values instead of just (ab)using one designed only for type registration.

Perhaps the code generator should use the lower level pgtype.CompositeBinaryBuilder and pgtype.CompositeBinaryScanner. It would probably be faster and it might just be simpler.

jschaf commented 3 years ago

Are you creating a CompositeType for each query?

Not quite. I'm creating a CompositeType for each composite type either used in query params or as an output column.

This would probably perform better than constructing a graph of CompositeTypes for each query. Since you're doing code generation it might even be simple for people using your tool.

Yea, I think that's probably the right way forward.