Add pgx driver package #517

dzbee · 2021-03-03T22:33:04Z

Closes #512

Changes:

Replace lib/pq dependency with pgx/stdlib in the postgres driver
Replace use of pq.QuoteIdentifier with private quoteIdentifier function in package
Slight change around parsing of cursor position from PgError (which is an int32 in pgx.PgError rather than string as in pq.PgError)

This PR would slightly change error reporting to include the SQL error code in the error message. If this is considered a backwards compatibility breaking change, I could add some parsing to omit the code so that this would only be a minor update rather than a major update, but I can't imagine that including additional information in the error message merits a major bump in the first place.

pgconn is a low-level library, and errors from queries flow through higher-level pgx library

coveralls · 2021-03-03T23:01:03Z

Pull Request Test Coverage Report for Build 183

188 of 290 (64.83%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.4%) to 56.547%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
database/pgx/pgx.go	188	290	64.83%

Totals
Change from base Build 143:	0.4%
Covered Lines:	3308
Relevant Lines:	5850

💛 - Coveralls

dhui

Thanks for the PR! Good to know that the smoke tests pass!
The different error messages may break existing postgres users. e.g. if they were handling said errors
Also, I believe WithInstance() may behave differently using pgx/stdlib instead of lib/pq, forcing users to migrate to pgx.

I'll hold off on merging this until there is more demand for it.

dhui · 2021-03-04T06:11:36Z

database/postgres/postgres.go

 	if _, err = p.conn.ExecContext(context.Background(), query); err != nil {
 		return &database.Error{OrigErr: err, Query: []byte(query)}
 	}

 	return nil
 }
+
+func quoteIdentifier(s string) string {


Use this implementation and reference it: https://github.com/lib/pq/blob/v1.9.0/conn.go#L1611

dzbee · 2021-03-04T17:16:05Z

@dhui With regards to concerns about creating conflicts for lib/pq users in the near-term, I wonder if you would consider relaxing this intention while the future of lib/pq vs pgx (or another replacement) remains unresolved:

...there should really just be one driver for a database...

To ease the transition from lib/pq to pgx I could split these changes out into a pgx.go and register the driver with that name instead of "postgres". Indeed lib/pq in some sense "owns" the "postgres" driver identifier whereas pgx/stdlib uses "pgx" to avoid conflicts on initialization in cases where both modules are imported. Given that the spirit of this rule is

We want to prevent a future where several drivers doing the exact same thing, just implemented a bit differently, co-exist somewhere on GitHub.

I don't think this change would contribute to such a future because lib/pq will eventually be disfavored in the community since it is not being actively developed.

Let me know what you think about this alternative.

dhui · 2021-03-05T03:23:43Z

Registering pgx as a separate migrate db driver is a great idea! e.g. following pgx/stdlib's lead
That way users can pick which postgres db driver to use. I'd hold off on recommending using pgx until the more of the community has moved over.

dhui

Tests are failing

dhui · 2021-03-06T06:34:19Z

database/pgx/README.md

@@ -0,0 +1,38 @@
+# postgres


Rename to pgx

dhui · 2021-03-06T06:34:30Z

database/pgx/README.md

@@ -0,0 +1,38 @@
+# postgres
+
+`postgres://user:password@host:port/dbname?query` (`postgresql://` works, too)


Update the DSN

database/pgx/README.md

database/pgx/pgx.go

dhui · 2021-03-09T17:50:27Z

go.mod

+	github.com/jackc/fake v0.0.0-20150926172116-812a484cc733 // indirect
+	github.com/jackc/pgconn v1.3.2
+	github.com/jackc/pgerrcode v0.0.0-20201024163028-a0d42d470451
+	github.com/jackc/pgx v3.6.2+incompatible


Looks like v4.10.1 is the latest release. Any reason why we don't use that?

That was simply a mistake resulting from the fact that pgx didn't support Go modules until v4. Therefore I was pulling the latest release prior to the introduction of modules into the project. The major version includes some breaking changes for the code I'd written before so I've included some updates with a version bump in def44ba.

Most significantly, in v3 the connection string parsing didn't include any checks on the content scheme name (see https://github.com/jackc/pgx/blob/a413de981978e4100d5736af7aaa2b392511612f/conn.go#L1246 and https://github.com/jackc/pgx/blob/a413de981978e4100d5736af7aaa2b392511612f/conn.go#L979) whereas now the postgres or postresql schemes are explicitly required in the connection string (see https://github.com/jackc/pgconn/blob/5daa019e4eb52df3409ebf17c83116b7c0e827e5/config.go#L199). Therefore I've introduced a string replacement so that after the pgx scheme is used to identify the appropriate driver in migrate, it's replaced with the appropriate postgres scheme for actually opening the connection.

Prior to v4, pgx did not support go modules. Version locking the import is the safest way to ensure we get the (as of now) latest version of the module. Additionally v4 implements some breaking changes to connection and error handling, which are also addressed in this commit.

dhui

What are the breaking changes in connection and error handling. It just looks like the connection errors were moved to another package.

dhui · 2021-03-11T07:02:28Z

database/pgx/pgx.go

+	// Driver is registered as pgx, but connection string must use postgres schema
+	// when making actual connection
+	// i.e. pgx://user:password@host:port/db => postgres://user:password@host:port/db
+	url = strings.Replace(url, "pgx://", "postgres://", 1)


Avoid manipulating URLs via string operations. Instead, set purl.Scheme.
I like the documentation in the comments though. Let's keep that!

dzbee · 2021-03-11T17:54:30Z

Connection errors were moved and connection string parsing was changed (#517 (comment)), but that's all. I just meant to explain why there were additional code changes accompanying the upgrade from v3 -> v4.

dhui

Thanks for the PR and addressing all of the feedback!
If you want pgx bundled in the CLI releases, you'll need to update DATABASE in the Makefile.

dzbee · 2021-03-12T15:24:55Z

Thanks for the tip, I definitely missed that. And thanks for working with me to get this added! It's a big help.

dhui · 2021-03-12T21:43:07Z

README.md

@@ -1,3 +1,23 @@
+<!-- START doctoc generated TOC please keep comment here to allow auto update -->


Please remove the generated TOC. If we add this, we're likely to use a different generator.

dzbee added 3 commits February 26, 2021 17:14

replace lib/pq dependency with pgx in postgres driver

995cc27

fix pgx error handling

420fede

pgconn is a low-level library, and errors from queries flow through higher-level pgx library

update postgres tests to use pgx driver and error reporting

0abe9fc

dzbee mentioned this pull request Mar 3, 2021

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

Closed

dhui reviewed Mar 4, 2021

View reviewed changes

update quoteIdentifier implementation to match lib/pq

7f32fe6

move changes for pgx support into new driver package

6c61cf3

dhui requested changes Mar 6, 2021

View reviewed changes

dzbee added 3 commits March 9, 2021 11:22

add pgx driver to CLI

98447ca

restore examples used in testing for pgx driver

2afbd8d

replace postgres driver name with pgx in DSN in tests and docs

db16491

dhui reviewed Mar 9, 2021

View reviewed changes

dzbee added 2 commits March 10, 2021 12:59

consolidate connection string formatting in pgx testing

611d4d1

dhui requested changes Mar 11, 2021

View reviewed changes

update parsed URL scheme instead of using string operation

d6e4514

dzbee changed the title ~~Replace lib/pq dependency with pgx in postgres driver~~ Add pgx driver package Mar 11, 2021

dhui previously approved these changes Mar 12, 2021

View reviewed changes

dzbee added 2 commits March 12, 2021 10:23

add pgx for builds using make

f2fa789

add pgx to top level docs

84b6e46

dzbee dismissed dhui’s stale review via 84b6e46 March 12, 2021 15:23

dhui requested changes Mar 12, 2021

View reviewed changes

remove autogenerated TOC

42470f2

dhui approved these changes Mar 14, 2021

View reviewed changes

dhui merged commit 2f0c075 into golang-migrate:master Mar 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pgx driver package #517

Add pgx driver package #517

dzbee commented Mar 3, 2021

coveralls commented Mar 3, 2021 •

edited

Loading

dhui left a comment

dhui Mar 4, 2021

dzbee commented Mar 4, 2021 •

edited

Loading

dhui commented Mar 5, 2021

dhui left a comment

dhui Mar 6, 2021

dhui Mar 6, 2021

dhui Mar 9, 2021

dzbee Mar 10, 2021 •

edited

Loading

dzbee Mar 10, 2021

dhui left a comment

dhui Mar 11, 2021

dzbee commented Mar 11, 2021

dhui left a comment

dzbee commented Mar 12, 2021

dhui Mar 12, 2021

		@@ -0,0 +1,38 @@
		# postgres

		`postgres://user:password@host:port/dbname?query` (`postgresql://` works, too)

		@@ -1,3 +1,23 @@
		<!-- START doctoc generated TOC please keep comment here to allow auto update -->

Add pgx driver package #517

Add pgx driver package #517

Conversation

dzbee commented Mar 3, 2021

coveralls commented Mar 3, 2021 • edited Loading

Pull Request Test Coverage Report for Build 183

💛 - Coveralls

dhui left a comment

Choose a reason for hiding this comment

dhui Mar 4, 2021

Choose a reason for hiding this comment

dzbee commented Mar 4, 2021 • edited Loading

dhui commented Mar 5, 2021

dhui left a comment

Choose a reason for hiding this comment

dhui Mar 6, 2021

Choose a reason for hiding this comment

dhui Mar 6, 2021

Choose a reason for hiding this comment

dhui Mar 9, 2021

Choose a reason for hiding this comment

dzbee Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

dzbee Mar 10, 2021

Choose a reason for hiding this comment

dhui left a comment

Choose a reason for hiding this comment

dhui Mar 11, 2021

Choose a reason for hiding this comment

dzbee commented Mar 11, 2021

dhui left a comment

Choose a reason for hiding this comment

dzbee commented Mar 12, 2021

dhui Mar 12, 2021

Choose a reason for hiding this comment

coveralls commented Mar 3, 2021 •

edited

Loading

dzbee commented Mar 4, 2021 •

edited

Loading

dzbee Mar 10, 2021 •

edited

Loading