Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make CompositeType Status public #100

Closed
wants to merge 1 commit into from

Conversation

jschaf
Copy link
Contributor

@jschaf jschaf commented Apr 23, 2021

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.

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
Copy link
Owner

jackc commented Apr 24, 2021

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
Copy link
Contributor Author

jschaf commented Apr 27, 2021

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.

  • A function to create an empty CompositeType.
  • A function to create an CompositeType and set its value.
  • A function to convert a composite type into []interface{} so we can use Set.

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

@jackc
Copy link
Owner

jackc commented Apr 30, 2021

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
Copy link
Contributor Author

jschaf commented May 4, 2021

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.

@jschaf jschaf closed this May 4, 2021
@jschaf jschaf deleted the joe/composite-public-status branch May 5, 2021 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants