pgx: Make it actually use pgx #593

AmandaCameron · 2021-06-30T18:44:44Z

This modifies the pgx driver to actually use pgx directly, and adds a WithPgxInstance function, which will let people who are using the pgx driver directly run migrations using the package as a library.

coveralls · 2021-06-30T19:22:46Z

Pull Request Test Coverage Report for Build 482

29 of 42 (69.05%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.03%) to 57.116%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
database/pgx/pgx.go	29	42	69.05%

Totals
Change from base Build 468:	-0.03%
Covered Lines:	3580
Relevant Lines:	6268

💛 - Coveralls

dhui · 2021-07-01T07:44:05Z

database/pgx/pgx.go

+	return px, nil
+}
+
+func WithPgxInstance(ctx context.Context, conn *pgx.Conn, config *Config) (database.Driver, error) {


Rename to WithConnection() and return (*Postgres, error) so you don't need to perform type assertions.
Also, do a compile time type check at the package level: var _ database.Driver = (*Postgres)(nil) // explicit compile time type check

dhui · 2021-07-01T07:44:16Z

database/pgx/pgx.go

-	// Locking and unlocking need to use the same connection
-	conn     *sql.Conn
-	db       *sql.DB
+	db *pgx.Conn


Why not keep the existing names? e.g.

type Postgres struct { conn *pgx.Conn db *sqll.DB }

Calling the connection db and the DB stdlib is confusing.

dhui · 2021-07-01T08:06:31Z

database/pgx/pgx.go

-	dbErr := p.db.Close()
-	if connErr != nil || dbErr != nil {
-		return fmt.Errorf("conn: %v, db: %v", connErr, dbErr)
+	if p.stdlib != nil {


We'll need to revisit this once the fields are renamed. The current field names make this logic hard to follow.

The pgx.Conn should be closed first and only released if the stdlib was used.

I'm under the impression that if we got it from AquireConn we're just "borowing" it from the sql.DB, so we shouldn't be closing it, but passing it back to the sql.DB instance with ReleaseConn, which is why I did it this way.

The docs for AcquireConn say:

AcquireConn acquires a *pgx.Conn from database/sql connection pool. It must be released with ReleaseConn.

Ahhh, that makes sense. e.g. if the connection is create/acquired via AcquireConn(), it should be returned/released via ReleaseConn() and not close. Similarly, database.sql.Conn.Raw() doesn't close connections and releases it when finished.

dhui · 2021-07-01T08:12:36Z

Thanks for the PR!

dhui

Thanks for addressing the PR feedback!

dhui · 2021-07-01T17:05:21Z

database/pgx/pgx.go

 	}
+
+	dbErr := p.conn.Close(context.Background())


There are 3 cases Close() needs to handle:

Created with Open()

p.db and p.conn are both non-nil

should release the connection and close the DB

Created with WithInstance()

p.db and p.conn are both non-nil

Should release the connection but leave the DB open for the caller of WithInstance() to close. However, this isn't the current behavior which is to close p.db, and existing clients may expect the current behavior, so let's leave it as is. e.g. close both p.db and release p.conn.

Created with WithConnection()

p.db is nil and p.conn is non-nil

Should leave the connection for the caller of WithConnection() to release the connection.

Per this comment, I think we should be releasing connection and never closing it

dhui · 2021-07-01T17:29:23Z

database/pgx/pgx.go

@@ -56,8 +58,9 @@ type Config struct {
 }

 type Postgres struct {
-	// Locking and unlocking need to use the same connection
-	conn     *sql.Conn
+	conn *pgx.Conn


Just realized that changing this a from a *sql.Conn to a *pgx.Conn will make it harder to factor out common db driver logic between the pgx and postgres drivers...

I don't see any direct pgx functionality being used in this PR. Are there any plans to do so?
Can we go back to using *sql.Conn? We can use Raw() or Conn() to get the underlying *pgx.Conn if necessary.

The benefit of using the pgx.Conn over sql.Conn is you can get a pgx.Conn from a sql.DB -- but you can't get a sql.DB from a pgx.Conn. This is the issue I was running into with a program of mine that uses pgx directly, for some speed benefits and some benefits around de/serialisation that can't be done through database/sql

That said, lib/pg's README states that it's in maintenance mode, and to use pgx instead, and I'm not aware of any other popular postgres go libraries.

Can a pgx.Conn be converted to a sql.Conn? I didn't see a way in the docs. In your project(s) can you get a sql.Conn even though you're using pgx?

That said, lib/pg's README states that it's in maintenance mode, and to use pgx instead, and I'm not aware of any other popular postgres go libraries.

migrate needs to continue supporting lib/pq since there are still active users even though it's in maintenance mode. There isn't much of a difference between the pgx and postgres drivers (diff the source code to see for yourself how trivial the differences are) and contributors shouldn't need to update both drivers when making changes.

The only way I could find to get a sql.Conn/DB in my program would be to open a separate connection using sql.Open instead of just passing in the pgx.Conn my program already created to do it's database operations, which just doesn't feel that clean to me.

Let's leave the pgx db driver as is for now until we have a usecase for using pgx.Conn directly. Hopefully, we'll have the postgres and pgx db drivers refactored by then... Thanks again for the PR!

Won't stdlib.OpenDB() create a new connection instead of using the existing one?

Yes but if a user who's using pgx.Conn everywhere else wants to call this library then they have to do that anyway

When a db driver is created with an existing connection, the user's expectation is for that connection to be used by migrate but managed by the user.

Ok, I see your point there.

I guess my qualm is just that, without this PR, pgx users that weren't otherwise using the stdlib compatibility layer need to import it and then open a connection with it themselves in ordert to pass it to NewWithDatabaseInstance. And if you're not using the WithDatabaseIstance entrypoint then it's still an awkward way in as afaict the URL-style connstrings aren't compatible due to migrate looking for a pgx:// scheme instead of postgres://, which pgx itself doesn't do. If you're already bought in to pgx and try to pick up migrate, it's tricky to figure out how to get the puzzle pieces to slot together. I think pushing for a unified pgx/pq driver that uses the sql.DB interface will leave that usecase unserved.

Ugh. Just ran into this ourselves. We're using pgx/v5 without the stdlib compat layer, and it seems like migrate doesn't really support using an existing pgx connection.

Not a deal breaker, but it would have been nice not having to duplicate a bunch of connection setup/handling code, just for the brief migrate connection. 😉

MattBrittan · 2023-11-10T04:13:42Z

FYI - pgx release 5.5 has added pool.OpenDBFromPool; not a perfect solution but it does avoid the need to open a separate connection (for those using pgx.Pool).

AmandaCameron added 2 commits June 30, 2021 14:41

pgx: Make it actually use pgx

a7a59af

pgx: Fix incorrect usage of sql.ErrNoRows

5f22944

dhui requested changes Jul 1, 2021

View reviewed changes

AmandaCameron added 2 commits July 1, 2021 08:25

Address MR comments.

f904d44

fix tests, use pgx for Postgres.Open as well.

9faafed

dhui requested changes Jul 1, 2021

View reviewed changes

AmandaCameron closed this Jul 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pgx: Make it actually use pgx #593

pgx: Make it actually use pgx #593

AmandaCameron commented Jun 30, 2021

coveralls commented Jun 30, 2021 •

edited

Loading

dhui Jul 1, 2021

dhui Jul 1, 2021

dhui Jul 1, 2021

AmandaCameron Jul 1, 2021

dhui Jul 1, 2021

dhui commented Jul 1, 2021

dhui left a comment

dhui Jul 1, 2021

dhui Jul 1, 2021

AmandaCameron Jul 1, 2021

dhui Jul 2, 2021

AmandaCameron Jul 2, 2021

dhui Jul 6, 2021

dhui Aug 5, 2021

treuherz Aug 6, 2021

dhui Aug 6, 2021

treuherz Aug 9, 2021

justinclift May 5, 2023

MattBrittan commented Nov 10, 2023

pgx: Make it actually use pgx #593

pgx: Make it actually use pgx #593

Conversation

AmandaCameron commented Jun 30, 2021

coveralls commented Jun 30, 2021 • edited Loading

Pull Request Test Coverage Report for Build 482

💛 - Coveralls

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

dhui commented Jul 1, 2021

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MattBrittan commented Nov 10, 2023

coveralls commented Jun 30, 2021 •

edited

Loading