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

Enhancing Custom Type Registration in pgx #2150

Open
codercms opened this issue Oct 18, 2024 · 2 comments
Open

Enhancing Custom Type Registration in pgx #2150

codercms opened this issue Oct 18, 2024 · 2 comments

Comments

@codercms
Copy link
Contributor

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:

pgx/derived_types.go

Lines 249 to 262 in 2ec9004

func serverVersion(c *Conn) (int64, error) {
serverVersionStr := c.PgConn().ParameterStatus("server_version")
serverVersionStr = regexp.MustCompile(`^[0-9]+`).FindString(serverVersionStr)
// if not PostgreSQL do nothing
if serverVersionStr == "" {
return 0, fmt.Errorf("Cannot identify server version in %q", serverVersionStr)
}
version, err := strconv.ParseInt(serverVersionStr, 10, 64)
if err != nil {
return 0, fmt.Errorf("postgres version parsing failed: %w", err)
}
return version, nil
}

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


func registerDefaultPgTypeVariants[T any](m *Map, name string) {
arrayName := "_" + name
var value T
m.RegisterDefaultPgType(value, name) // T
m.RegisterDefaultPgType(&value, name) // *T
var sliceT []T
m.RegisterDefaultPgType(sliceT, arrayName) // []T
m.RegisterDefaultPgType(&sliceT, arrayName) // *[]T
var slicePtrT []*T
m.RegisterDefaultPgType(slicePtrT, arrayName) // []*T
m.RegisterDefaultPgType(&slicePtrT, arrayName) // *[]*T
var arrayOfT Array[T]
m.RegisterDefaultPgType(arrayOfT, arrayName) // Array[T]
m.RegisterDefaultPgType(&arrayOfT, arrayName) // *Array[T]
var arrayOfPtrT Array[*T]
m.RegisterDefaultPgType(arrayOfPtrT, arrayName) // Array[*T]
m.RegisterDefaultPgType(&arrayOfPtrT, arrayName) // *Array[*T]
var flatArrayOfT FlatArray[T]
m.RegisterDefaultPgType(flatArrayOfT, arrayName) // FlatArray[T]
m.RegisterDefaultPgType(&flatArrayOfT, arrayName) // *FlatArray[T]
var flatArrayOfPtrT FlatArray[*T]
m.RegisterDefaultPgType(flatArrayOfPtrT, arrayName) // FlatArray[*T]
m.RegisterDefaultPgType(&flatArrayOfPtrT, arrayName) // *FlatArray[*T]
}

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:

pgx/derived_types.go

Lines 158 to 163 in 2ec9004

// LoadTypes performs a single (complex) query, returning all the required
// information to register the named types, as well as any other types directly
// or indirectly required to complete the registration.
// The result of this call can be passed into RegisterTypes to complete the process.
func (c *Conn) LoadTypes(ctx context.Context, typeNames []string) ([]*pgtype.Type, error) {
m := c.TypeMap()

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

jackc commented Oct 22, 2024

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

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

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

No branches or pull requests

2 participants