Removes pgx dependency #15

aravindc26 · 2017-07-28T09:41:21Z

Since go 1.9's database/sql interface allows you grab an individual connection, I have removed pgx dependency.

cbandy

Looks good. If I understand correctly, sql.Conn is part of Go 1.9, yes?

cbandy · 2017-07-28T12:41:52Z

enqueue_test.go

-	// truncate to the microsecond as postgres driver does
-	want = want.Truncate(time.Microsecond)
+	// truncate to the Second as postgres driver does
+	want = want.Truncate(time.Second)


Why this change?

cbandy · 2017-07-28T12:46:52Z

que.go

-		Time:  j.RunAt,
-		Valid: !j.RunAt.IsZero(),
+	runAt := sql.NullString{
+		String: j.RunAt.Format(time.RFC3339),


Can use time.RFC3339Nano to include microseconds here.

@cbandy yes I'll change it. If time.RFC3339Nano is used here, I guess there is no need of truncating it to second in the tests.

cbandy · 2017-07-28T13:15:34Z

work_test.go

 		conn := newConn()
 		i := 0
 		for {
 			var waiting bool
-			err := conn.QueryRow(`SELECT pg_stat_get_backend_waiting($1)`, backendID).Scan(&waiting)
+			err := conn.QueryRowContext(context.Background(), `SELECT waiting from pg_stat_activity where pid=$1`, backendID).Scan(&waiting)


Why this change?

I am using postgresql 9.6 and pg_stat_get_backend_waiting doesn't seem to exist in 9.6.

cbandy · 2017-07-28T13:19:48Z

que.go


 	mu      sync.Mutex
 	deleted bool
-	pool    *pgx.ConnPool
-	conn    *pgx.Conn
+	db      *sql.DB


Personally, I don't mind this still being named pool. Newcomers to the database/sql package often do not recognize that *sql.DB is a connection pool. (Keeping it as pool would also reduce the size of this diff.)

cbandy · 2017-07-28T13:21:14Z

que.go

@@ -121,14 +121,14 @@ func (j *Job) Error(msg string) error {
 // Client is a Que client that can add jobs to the queue and remove jobs from
 // the queue.
 type Client struct {
-	pool *pgx.ConnPool
+	db *sql.DB


I'm okay with pool here as well.

cbandy · 2017-07-28T13:26:40Z

que.go

@@ -121,14 +121,14 @@ func (j *Job) Error(msg string) error {
 // Client is a Que client that can add jobs to the queue and remove jobs from
 // the queue.
 type Client struct {
-	pool *pgx.ConnPool
+	db *sql.DB

 	// TODO: add a way to specify default queueing options
 }

 // NewClient creates a new Client that uses the pgx pool.


I would remove "pgx" here.

... that uses the connection pool.

cbandy · 2017-07-28T13:28:36Z

que.go

-	pool    *pgx.ConnPool
-	conn    *pgx.Conn
+	db      *sql.DB
+	conn    *sql.Conn
 }

 // Conn returns the pgx connection that this job is locked to. You may initiate


I would remove "pgx" here.

Conn returns the connection that this job ...

bgentry · 2017-07-28T17:24:41Z

@aravindc26 thanks for submitting this! I have a few questions before I review it.

Does this have any semantic differences that pre-existing users will need to adjust for when they migrate over? It looks like pgx/stdlib has a DriverConfig struct that's basically the same as the current pgx.ConnPoolConfig but used to configure the database/sql driver. Is there any reason that direct pgx support might need to be preserved? (If others can chime in here that would be 🙏)
Can you update the readme both to reflect the new usage?
Can you also add a readme note that explains how existing users should migrate from older versions?

I realized I hadn't yet tagged any releases on this repo, so I just tagged v0.5.0 on master. I think after this PR I'll want to bump it version to 1.0 since this is a breaking change.

aravindc26 · 2017-07-31T09:01:03Z

Does this have any semantic differences that pre-existing users will need to adjust for when they migrate over? It looks like pgx/stdlib has a DriverConfig struct that's basically the same as the current pgx.ConnPoolConfig but used to configure the database/sql driver. Is there any reason that direct pgx support might need to be preserved? (If others can chime in here that would be 🙏)

@bgentry There are some differences in API between database/sql and pgx's native driver. One such difference is that database/sql doesn't seem to support named prepared statements (https://tip.golang.org/pkg/database/sql/#Conn.PrepareContext), while there is support for named prepared statements in pgx native lib (https://godoc.org/github.com/jackc/pgx#Conn.Prepare); as a result I had to remove the PrepareQuery API from que-go and use the queries without preparation and I am not sure about the performance impact.

Completely removing pgx support would mean that one cannot use ConnPool's AfterConnect feature. In a project that I work, I use AfterConnect to set search_path, this is useful when the que_jobs table is not created in public schema. May be we could come up with a solution to overcome the reliance on AfterConnect ?

Can you update the readme both to reflect the new usage?
Can you also add a readme note that explains how existing users should migrate from older versions?

Sure I can work on them 👍

bgentry · 2017-07-31T16:56:00Z

@bgentry There are some differences in API between database/sql and pgx's native driver. One such difference is that database/sql doesn't seem to support named prepared statements (https://tip.golang.org/pkg/database/sql/#Conn.PrepareContext), while there is support for named prepared statements in pgx native lib (https://godoc.org/github.com/jackc/pgx#Conn.Prepare); as a result I had to remove the PrepareQuery API from que-go and use the queries without preparation and I am not sure about the performance impact.

If you're saying that you've entirely removed the use of prepared statements in this PR (which is what it appears like at a glance), that's probably a no-go. When I was writing this lib, there was a dramatic difference in perf once prepared statements were added (likely bc the Que SQL statements are fairly complex).

aravindc26 · 2017-08-01T14:00:38Z

@bgentry if there is dramatic difference in performance, it does not make sense to move to database/sql; Closing the PR.

aravindc26 · 2017-08-01T14:00:54Z

Would you accept a PR if I were to upgrade the pgx dependency to v3 ?

bgentry · 2017-08-01T15:16:33Z

Absolutely!

…

On Tue, Aug 1, 2017 at 7:01 AM Aravind Chintalapalli < ***@***.***> wrote: Would you accept a PR if I were to upgrade the pgx dependency to v3 ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAG9cTzF2grPBFYdQEwZFKirWk40rqmgks5sTy-XgaJpZM4OmY4o> .

Removes pgx dependency

c814d91

cbandy reviewed Jul 28, 2017

View reviewed changes

aravindc26 closed this Aug 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removes pgx dependency #15

Removes pgx dependency #15

aravindc26 commented Jul 28, 2017

cbandy left a comment

cbandy Jul 28, 2017

cbandy Jul 28, 2017

aravindc26 Jul 31, 2017

cbandy Jul 28, 2017

aravindc26 Jul 31, 2017

cbandy Jul 28, 2017

aravindc26 Jul 31, 2017

cbandy Jul 28, 2017

cbandy Jul 28, 2017

cbandy Jul 28, 2017

aravindc26 Jul 31, 2017

bgentry commented Jul 28, 2017

aravindc26 commented Jul 31, 2017

bgentry commented Jul 31, 2017

aravindc26 commented Aug 1, 2017

aravindc26 commented Aug 1, 2017

bgentry commented Aug 1, 2017 via email

Removes pgx dependency #15

Removes pgx dependency #15

Conversation

aravindc26 commented Jul 28, 2017

cbandy 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

bgentry commented Jul 28, 2017

aravindc26 commented Jul 31, 2017

bgentry commented Jul 31, 2017

aravindc26 commented Aug 1, 2017

aravindc26 commented Aug 1, 2017

bgentry commented Aug 1, 2017 via email