jackc / pgx

PostgreSQL driver and toolkit for Go
MIT License
10.83k stars 845 forks source link

Enhancing Custom Type Registration in pgx #2150

Open codercms opened 1 month ago

codercms commented 1 month ago

I've created PostgreSQL extension that adds native unsigned integer data types to PostgreSQL. I also implemented support of these types in pgx.

However implementing pgx part was a bit painful because I have need to copy & paste pgx internals code:

https://github.com/jackc/pgx/blob/2ec900454bfe65daa9648488e93f7627c26b810c/derived_types.go#L249-L262 https://github.com/pg-uint/pgx-pg-uint128/blob/d194dc5568275d1aabc0a6900f4311012e21abdf/types/register.go#L241-L257


https://github.com/jackc/pgx/blob/2ec900454bfe65daa9648488e93f7627c26b810c/pgtype/register_default_pg_types.go#L5-L35 https://github.com/pg-uint/pgx-pg-uint128/blob/d194dc5568275d1aabc0a6900f4311012e21abdf/types/register_default_pg_types.go#L31-L62


And there's also lack of ability to register base types: https://github.com/jackc/pgx/blob/2ec900454bfe65daa9648488e93f7627c26b810c/derived_types.go#L158-L163

So, I have need need to implement types registration on my own: https://github.com/pg-uint/pgx-pg-uint128/blob/d194dc5568275d1aabc0a6900f4311012e21abdf/types/register.go#L66-L122

https://github.com/pg-uint/pgx-pg-uint128/blob/d194dc5568275d1aabc0a6900f4311012e21abdf/types/register.go#L124-L130

To streamline the process, I suggest we export some of these functions, such as registerDefaultPgTypeVariants and serverVersion (perhaps as a member method of conn).

Additionally, we could either adapt the LoadTypes function to support loading base types or provide a new API for base type registration.

jackc commented 1 month ago

You've got a lot of functionality in those packages. Just out of curiosity, what's the domain you're working in?


As far as adding more public interfaces, I can see how it would be very useful in your particular case. But I'm not so sure if any of those internal functions are things I want to support.

serverVersion only provides the major version of an actual PostgreSQL server. But if was a public function I would expect it to in some manner handle minor versions and non-PostgreSQL servers such as CockroachDB.

I don't have any concrete reason for not exposing registerDefaultPgTypeVariants, aside from it just seems like an internal helper function, not something I want to document and support.

I don't know, I'm open to reconsidering, but I think copy and paste might actually be the best approach for the above-mentioned functions.

As far as better supporting loading base types either through LoadTypes or a new API, I think that is reasonable.

codercms commented 1 month ago

Just out of curiosity, what's the domain you're working in?

I found that a popular PostgreSQL extension for unsigned integer data types is no longer maintained, so I decided to implement my own version for learning and experience. The key benefit of using unsigned integers in a database is that you can store values in their native representation (when you deal with uint's of course), avoiding the need for higher precision signed integers or dealing with incorrect representation in same size signed integers (like negative values on overflow) and conversion issues on the application side.

For example, you can store a port number in uint2 (16-bit unsigned integer), but you can't store it in int2 without overflow. This leaves you with two options: casting uint16 to int16 (which causes overflow) or using int4 (int32) instead.


serverVersion

Yep, I see your point here, probably it's not the best place to maintain such function inside the driver, because once it becomes public it should cover all edge cases

registerDefaultPgTypeVariants

I agree, probably future changes in this helper function behavior could break applications relying on the older behavior. So it's better to keep it internal.

The only thing left to think about base types loading support, I'll research if there are more use cases for it