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

Replace lib/pq with pgx/stdlib as postgres driver #512

Closed
dzbee opened this issue Feb 26, 2021 · 3 comments · Fixed by #517
Closed

Replace lib/pq with pgx/stdlib as postgres driver #512

dzbee opened this issue Feb 26, 2021 · 3 comments · Fixed by #517

Comments

@dzbee
Copy link
Contributor

dzbee commented Feb 26, 2021

Is your feature request related to a problem? Please describe.

The current postgres driver used by migrate is lib/pq, which is currently in maintenance mode. The maintainers of lib/pq encourage users to change over to pgx. Beside the fact that lib/pq will eventually be disfavored relative to pgx in the community (if it's not already), dependence on the lib/pq driver has blocked our org from upgrading to Go 1.15 (see outstanding lib/pq branch), and we'd like to remove the lib/pq dependency while continuing to use migrate.

Describe the solution you'd like

I propose that pgx/stdlib replace the lib/pq driver for providing postgres support for migrate, in the spirit of the outlook outlined in the FAQ.

Describe alternatives you've considered

Our current solution in our org is to perform the database migration with migrate and open a separate long-lived database connection for app support with the pgx/stdlib driver to limit the impact of the lib/pq dependency. But in the long term, the community undoubtedly will have to move away from lib/pq entirely.

@dhui
Copy link
Member

dhui commented Feb 27, 2021

Thanks for raising this issue!

Is pgx/stdlib a drop-in replacement for lib/pg?
Do you know if lib/pq and pgx/stdlib support the same DSN?

I see these pgx swapping issues, which gives me pause to switch:

@dzbee Have you tried switching out the driver and running tests?

Does anyone want to stay on lib/pq?
I'm hesitant to switch until we gather more data.

@dzbee
Copy link
Contributor Author

dzbee commented Mar 1, 2021

Replacing the driver itself is as simple as swapping the package and changing the string identifier for the driver in sql.Open. Also pgx/stdlib supports standard DSN and has replacement functions for lib/pq environment parsing:
https://pkg.go.dev/gopkg.in/jackc/pgx.v3#ParseDSN
https://pkg.go.dev/gopkg.in/jackc/pgx.v3#ParseConnectionString
https://pkg.go.dev/gopkg.in/jackc/pgx.v3#ParseURI
https://pkg.go.dev/gopkg.in/jackc/pgx.v3#ParseEnvLibpq

There are a few other places where lib/pq is used in the postgres driver for migrate related to error handling and query building, but I don't believe those changes are very complicated either. You can view a diff of a speculative branch I made here: master...nicheinc:feature/pgx-support. That branch is untested at the moment, but I believe it illustrates the (limited) extent of the changes required. I'll report back again once I've had a chance to test and fix any bugs.

I wanted to respond to some of the issues you linked in the meantime:

  1. PLEASE REMOVE THE ADVICE TO USE PGX FROM YOUR PAGE lib/pq#1022 -- I'm not sure what this commentor is talking about with regards to lack of float64 support; reading through the linked issues, the pgx maintainers don't acknowledge that "PGX can't handle float64" as the commentor claims (as another commentor explained, the proper scanner type for float64 is pgtype.Float8), but rather pgx is seemingly more strict about enforcing type agreement than lib/pq is. The types defined in pgtype are also easily extensible, seeing as how the scanner interface is fairly simple. As for the connection pool issue the commentor raises, it appears the issue was resolved in 2019.
  2. Why is this in maintenance mode? lib/pq#1010 -- I don't think this issue has significant implications for migrate since it sounds like timezones are handled by the client, not the database, so there as far as I can tell there should be no implication for schema migrations. The issue raised about reconnects should be handled in pgx/stdlib: Database restarts are not handled gracefully jackc/pgx#672 (comment)
  3. Unnecessary dependencies for non-Kerberos users lib/pq#971 (comment) -- In my speculative branch, it appears that only two additional dependencies, gofrs/uuid and jackc/fake, are introduced besides the direct pgx imports. Unless I messed something up, when I built the binary there was no appreciable change in size. Also from my reading of the linked issue, it appears the complaint about increased binary size is not due to pgx dependencies, but rather because of Kerberos support added to lib/pq, and the commentor was merely drawing a distinction between dependencies in lib/pq and pgx, not saying that pgx caused their binaries to grow

@dzbee
Copy link
Contributor Author

dzbee commented Mar 3, 2021

I've confirmed that the happy path is working for our use case of using a file source for migrating a postgres database in app using https://github.com/nicheinc/migrate/tree/feature/pgx-support, and with minor updates to the expected error reporting in testing (in particular to include the SQL error code), that branch is also passing testing. I've opened up a PR: #517

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 a pull request may close this issue.

2 participants