Repository navigation
Add support for pgx #472
Description
Activity
We currently use PGX and its batching mechanism. Would be great if you keep pgx batching in mind while this unfolds.
@ldelossa Do you have an example of the batching API? This is the first I've heard of it
@kyleconroy we wrap it a bit to handle variable sized input:
https://github.com/quay/claircore/blob/master/pkg/microbatch/microbatch.gousage:
https://github.com/quay/claircore/blob/master/internal/indexer/postgres/indexpackage.go#L96Reacted by Mikhail Kalinin and Hong Truongpgx is new to me, so I wanted to spend some time getting the driver working for the example in
examples/booktest/postgresql/. Here's a quick diff of that: powersjcb#1Some learnings:
- Several fields in
pgtypedo not implementMarshalJSON. (at leastpgtype.Timestampandpgtype.VarcharArray) So at first glance, it looks like fields wont be compatible with theemitjsontagsfeature of sqlc. - Many fields need to be initialized and then have a value Set into them using the following signature
(dst *Timestamp) Set(src interface{}) error. User application code will no longer be able to type check these inputs at compile time. For exampleerr := pgtype.Timestamp{}.Set("asdf")will compile without errors.
- Several fields in
Awesome!
It would be cool to support decoding composite types at some point. It's pretty tedious to implement
DecodeBinarysince you have to pull out every value from thepgtype.Recordby hand. Since we know the schema that could be automated and save a lot of time.* Several fields in `pgtype` do not implement `MarshalJSON`. (at least `pgtype.Timestamp` and `pgtype.VarcharArray`) So at first glance, it looks like fields wont be compatible with the `emitjsontags` feature of sqlc.In the booktest example at least, the
availablecolumn usestimestamptzwhosepgtypecounterpartTimestamptzdoes support JSON. Regardless I'm not sure it's a problem that raw timestamps don't have a JSON encoding. JSON times always have a zone and Postgres timestamps are ambiguous about what zone they're in so there's no natural conversion.Do you really need to support pgtype... There are a lot of times you don't need it.. and pointer to native type is enough. This also means that you can use the same struct through most application and there is no need to convert between db struct and "domain" struct. The onl conversion is then for viewing purposes.
Thanks for the feedback @mvrhov! I was able to get my test cases working with some native go types and the pgx driver interfaces.
Also, just discovered that sqlc only implements 1-dimensional array types, so that will help keep things simple. 👍
Reacted by Miha Vrhovnik and Yvan da SilvaI noticed we're still using
pq.Arrayfor postgres array type (e.g.TEXT[]becomespq.Array([]string)). Is it possible for these types to map to the pgx types instead (i.e.TEXT[]becomespgtype.TextArray)? I tried using an override and it didn't seem to work in 1.4.0.Reacted by Said Saifi3 remaining items
@kyleconroy Thanks for explaining.
It would be a great improvement to fully support pgx, can't wait for it!Reacted by Wyatt Arent, NaLLiFFuNT, Mikhail Kalinin, Mehmet Esen, Aleksandr Baryshnikov, EmilLaursen, natan streppel and VladyslavHey guys! @kyleconroy do we have any updates on this? I've just hit this wall unfortunately.
Edit: I'm creating a fork right now with a colleague and we'll try to work this out for our case (
pgxpool.Pool) as we need to deliver something for next week, if we're able to get this fixed I'll let you know hereReacted by Steve Coffman, Aleksandr Baryshnikov, Mehmet Esen, Mikhail Kalinin, Joe F and omid9hHey everyone, just in time for christmas
I ended up doing something that worked out fine for me. You can check what's different in this diff.
Usage remains equal, except that you need to inform a new parameter at the startup configuration yml file, as in
version: "1" packages: sql_library: "pgx/v4"
If this parameter is missing it defaults to the old behavior. Currently only supporting :one, :many, :exec (still need to implement others commands and tags)
I'm not very fond of this name I've used but couldn't think of anything better at the time. From my restricted testing everything looks fine to me, however I didn't do anything fancy yet. Maybe this could be a start.
Edit: to check it yourself, download my fork, switch branches to
feat/pgx_pool_support, compile it and install it locally following the README instructions and test it!Reacted by Aleksandr Baryshnikov, Mario Carrion, Mikhail Kalinin, Natan Albuquerque, Paul, Wyatt Arent, Kevin Burke, EmilLaursen, Ryan, Alexandru Rosianu and 2 moreReacted by Andrew Bagshaw, Ryan, Mehmet Esen, Victor and Dan ClipcaReacted by Vladyslav and Dan Clipca@kyleconroy and others: even though the above fits my use case (which is indeed very simple at this moment) it still is very premature in many aspects to even consider opening a pull request. Despite that, do you think the changes made here are heading in the right direction? If so, we could maybe merge this on a new development branch and continue work over there
Reacted by Steve CoffmanJust following up here - found another data race in lib/pq and it would be great to have a chance to evaluate other drivers, but at the moment we're pretty tied to using sqlc.
@kevinburkemeter pgx/stdlib works ok with sqlc.
Reacted by Andrew Bagshaw, Andrew, Vladyslav and James Quallsworking on that. is there a currently supported way to replace
pq.Array?afaik you don't need it for standard types.
pq.Arrayis part of github.com/lib/pq, a competing SQL driver, not part of the standard library, so it seems unlikely pgx would support itpq.Arrayimplements thedatabase/sql/driver.Valueranddatabase/sql.Scannerinterfaces, so it works fine withpgxin my testing. It's a bit messy to import both, but it works.Did #1037 replace the use of
pq.Arrayfor postgres arrays? Otherwise I wouldn't consider this issue completely closed.Reacted by Mehmet Esen, Moni and Steve Coffman@johanbrandhorst Is it possible to generate
pgtype.TextArray? I'm fighting with source codes and sqlc.yaml couldn't find a way yet.I don't think is it, yet.
Reacted by Mehmet Esen
Get out the trumpets and ready the 21-gun salute,
lib/pqis deprecated (#470). This means it's time to support it's successor, pgx.This will be the main tracking issue for pgx. It supersedes #28, as I have no intention of adding support for any additional PostgreSQL drivers beyond pgx.