Add driver for pgx v5 #848

treuherz · 2022-11-01T16:20:36Z

Fixes #827

Move existing driver into database/pgx/v4
Make exported items into database/pgx into links to database/pgx/v4,
marked as Deprecated
Mark defunct ErrDatabaseDirty as Deprecated - no code returns it
Bump pgx/v4 dependency
Add test runs for newer Postgres versions

If you'd like the last couple of changes to be broken into a different PR then let me know.

coveralls · 2022-11-01T16:53:20Z

Coverage: 58.526%. Remained the same when pulling 1ecd0a8 on treuherz:feature/pgxv5 into 8ec0422 on golang-migrate:master.

koenbollen · 2022-12-14T15:37:19Z

Hey! Quick bump from me? I'm currently running this branch to allow me to use pgx and golang-migrate together. (thanks btw!)

Any updates on getting this merged? 😇

treuherz · 2022-12-16T14:51:40Z

Rebased on top of master

mprimeaux · 2023-01-07T01:37:14Z

Any updates on getting this merged? Appreciate everyone's time.

pmorelli92 · 2023-01-25T13:48:30Z

Please some prio 🙏

franchb · 2023-02-05T07:00:53Z

Any updates on this PR please?

MNovruzov · 2023-02-20T12:37:34Z

Any updates please???

mprimeaux · 2023-03-04T18:48:22Z

Hi @dhui. Would you mind reviewing and consider an approval? We have quite a few platforms waiting for this PR to be merged. Your help, and the help of any maintainer, is greatly appreciated.

treuherz · 2023-03-14T09:37:09Z

Rebased on top of master again to fix conflicts, and bumped pgx minor versions again.

@vincentdaniel it was nice of you to approve this last time, but it didn't look like that granted mergeability, is that correct?

vincentdaniel · 2023-03-14T09:42:02Z

@treuherz correct, I am not maintainer of the project but I tried my luck anyway to see 🙈

dhui

@treuherz thanks for the PR and apologies for the slow turnaround time. Life has been busy...

I'll also update golang-migrate to only support (and run tests for) Go 1.19 and 1.20

dhui · 2023-03-15T05:22:35Z

database/pgx/pgx.go

-var (
-	multiStmtDelimiter = []byte(";")
-
-	DefaultMigrationsTable       = "schema_migrations"


These would need to be exported as well.
I think it's safer and easier to leave the default as v4 (e.g. remove the v4 folder in this PR) and update the README and godocs to also mention that a v5 driver is also available. I think it's fine to offer both v4 and v5 so we can mark v4 as deprecated later. e.g. whenever pgx deprecates v4. Do you know if v4 has already been deprecated?

Ok, I can do that.

Depends what you mean by deprecated. I think jackc is still applying critical bugfixes to v4 but it's not having significant work done.

dhui · 2023-03-15T05:27:42Z

database/pgx/v5/README.md

@@ -0,0 +1,41 @@
+# pgx
+
+Backends are provided for [pgx v4](v4) and [pgx v5](v5). Links to v4 are exported in this package for backwards compatability.


This will need to be updated

dhui · 2023-03-15T05:32:49Z

internal/cli/build_pgx.go

@@ -4,5 +4,5 @@
 package cli

 import (
-	_ "github.com/golang-migrate/migrate/v4/database/pgx"
+	_ "github.com/golang-migrate/migrate/v4/database/pgx/v5"


Fixes golang-migrate#827 - Move existing driver into database/pgx/v4 - Make exported items into database/pgx into links to database/pgx/v4, marked as Deprecated - Remove most tests in database/pgx, leaving a couple of e2e tests behind to help ensure backwards compatibility. - Mark defunct ErrDatabaseDirty as Deprecated - no code returns it - Bump pgx/v4 dependency - Add test runs for newer Postgres versions

treuherz · 2023-03-17T17:07:21Z

Implemented comments, rebased on master again to resolve go.mod conflicts.

dhui

@treuherz Thanks for the PR updates. This should be the last round.

dhui · 2023-03-17T22:39:37Z

README.md

@@ -24,7 +24,8 @@ Forked from [mattes/migrate](https://github.com/mattes/migrate)
 Database drivers run migrations. [Add a new database?](database/driver.go)

 * [PostgreSQL](database/postgres)
-* [PGX](database/pgx)
+* [PGX v4](database/pgx/v4)


This should be database/pgx

dhui · 2023-03-17T22:44:21Z

database/pgx/v5/pgx.go

+
+func init() {
+	db := Postgres{}
+	database.Register("pgx", &db)


This should be registered as pgx5 to avoid a conflict with pgx (v4)

As of a few weeks ago, pgx registers its database/sql Driver under pgx/v4 or pgx/v5, as well as conditionally registering pgx if it’s not already registered by importing another version (see https://github.com/jackc/pgx/blob/9ae852eb583d2dced83b1d2ffe1c8803dda2c92e/stdlib/sql.go#L94). I’m happy to copy in the conflict-avoidance code or if you prefer just leave it out and have v4 register as pgx and pgx/v4, and v5 only register pgx/v5

I definitely think it would be better to match pgx’s pgx/vN registration string rather than inventing a new one. I know I didn’t use it in this PR originally but the upstream change is very new!

dhui · 2023-03-17T22:48:02Z

internal/cli/build_pgxv5.go

@@ -0,0 +1,8 @@
+//go:build pgxv5


Can you rename this file and tag to pgx5? The v is verbose and unnecessary.

dhui · 2023-03-17T22:55:07Z

Makefile

@@ -1,5 +1,5 @@
 SOURCE ?= file go_bindata github github_ee bitbucket aws_s3 google_cloud_storage godoc_vfs gitlab
-DATABASE ?= postgres mysql redshift cassandra spanner cockroachdb yugabytedb clickhouse mongodb sqlserver firebird neo4j pgx
+DATABASE ?= postgres mysql redshift cassandra spanner cockroachdb yugabytedb clickhouse mongodb sqlserver firebird neo4j pgx pgxv5


You'll also need to update this to pgx5

dhui

@treuherz Could you also update the db registration name?

dhui · 2023-03-25T06:03:56Z

database/pgx/pgx.go

@@ -28,6 +28,7 @@ import (
 func init() {
 	db := Postgres{}
 	database.Register("pgx", &db)
+	database.Register("pgx/v4", &db)


Register this as pgx4 since pgx/v4 is verbose. We don't need to be consistent with the upstream. Also, our DSN/connection string parsing uses URL parsing and / is not a valid scheme character

dhui · 2023-03-25T06:05:03Z

database/pgx/v5/pgx.go

@@ -27,7 +27,7 @@ import (

 func init() {
 	db := Postgres{}
-	database.Register("pgx", &db)
+	database.Register("pgx/v5", &db)


Register as pgx5 for the same reason

kriswill · 2023-04-03T18:17:28Z

can we merge/release this?

dhui

@treuherz Thanks again for the PR and for addressing my feedback!

treuherz force-pushed the feature/pgxv5 branch from 54dbdb8 to 402b5ef Compare November 1, 2022 18:08

treuherz force-pushed the feature/pgxv5 branch from 402b5ef to 456a5f2 Compare December 16, 2022 14:51

treuherz force-pushed the feature/pgxv5 branch from 456a5f2 to c198cf3 Compare January 26, 2023 13:39

vincentdaniel previously approved these changes Feb 15, 2023

View reviewed changes

treuherz dismissed vincentdaniel’s stale review via f0c09d2 March 14, 2023 09:35

treuherz force-pushed the feature/pgxv5 branch from c198cf3 to f0c09d2 Compare March 14, 2023 09:35

treuherz force-pushed the feature/pgxv5 branch from f0c09d2 to 757c8a2 Compare March 14, 2023 09:43

dhui requested changes Mar 15, 2023

View reviewed changes

treuherz mentioned this pull request Mar 17, 2023

Question: migrate or implement pgx v5 driver #827

Closed

treuherz force-pushed the feature/pgxv5 branch from 757c8a2 to e1531fa Compare March 17, 2023 17:01

treuherz force-pushed the feature/pgxv5 branch from e1531fa to f1ee109 Compare March 17, 2023 17:06

treuherz requested a review from dhui March 17, 2023 17:07

dhui requested changes Mar 17, 2023

View reviewed changes

Tweak pgx tags and registration strings

fcee15f

dhui requested changes Mar 25, 2023

View reviewed changes

Tweak registration strings

1ecd0a8

dhui approved these changes Apr 5, 2023

View reviewed changes

dhui merged commit b94ed1a into golang-migrate:master Apr 5, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add driver for pgx v5 #848

Add driver for pgx v5 #848

treuherz commented Nov 1, 2022

coveralls commented Nov 1, 2022 •

edited

koenbollen commented Dec 14, 2022

treuherz commented Dec 16, 2022

mprimeaux commented Jan 7, 2023

pmorelli92 commented Jan 25, 2023

franchb commented Feb 5, 2023

MNovruzov commented Feb 20, 2023

mprimeaux commented Mar 4, 2023

treuherz commented Mar 14, 2023

vincentdaniel commented Mar 14, 2023

dhui left a comment

dhui Mar 15, 2023

treuherz Mar 17, 2023

dhui Mar 15, 2023

dhui Mar 15, 2023

treuherz commented Mar 17, 2023 •

edited

dhui left a comment

dhui Mar 17, 2023

dhui Mar 17, 2023

treuherz Mar 19, 2023 •

edited

dhui Mar 17, 2023

dhui Mar 17, 2023

dhui left a comment

dhui Mar 25, 2023

treuherz Apr 4, 2023

dhui Mar 25, 2023

treuherz Apr 4, 2023

kriswill commented Apr 3, 2023

dhui left a comment

		@@ -0,0 +1,41 @@
		# pgx

		Backends are provided for [pgx v4](v4) and [pgx v5](v5). Links to v4 are exported in this package for backwards compatability.

Add driver for pgx v5 #848

Add driver for pgx v5 #848

Conversation

treuherz commented Nov 1, 2022

coveralls commented Nov 1, 2022 • edited

koenbollen commented Dec 14, 2022

treuherz commented Dec 16, 2022

mprimeaux commented Jan 7, 2023

pmorelli92 commented Jan 25, 2023

franchb commented Feb 5, 2023

MNovruzov commented Feb 20, 2023

mprimeaux commented Mar 4, 2023

treuherz commented Mar 14, 2023

vincentdaniel commented Mar 14, 2023

dhui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

treuherz commented Mar 17, 2023 • edited

dhui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

treuherz Mar 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriswill commented Apr 3, 2023

dhui left a comment

Choose a reason for hiding this comment

coveralls commented Nov 1, 2022 •

edited

treuherz commented Mar 17, 2023 •

edited

treuherz Mar 19, 2023 •

edited