-
Notifications
You must be signed in to change notification settings - Fork 167
chore/fix(sql): remove lib/pq in favor of pgx #577
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| @@ -0,0 +1,18 @@ | |||
| package postgrespgx | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this driver approach? Can we not just directly swap out lib/pq usage with "github.com/jackc/pgx/v4/stdlib"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, the pgx driver is just not called postgres, it's called pgx or pgx/vX:
https://github.com/jackc/pgx/blob/ae204a414cbdd6fdb503808221dda22335cf4fd9/stdlib/sql.go#L107-L117
func init() {
pgxDriver = &Driver{
configs: make(map[string]*pgx.ConnConfig),
}
// if pgx driver was already registered by different pgx major version then we
// skip registration under the default name.
if !slices.Contains(sql.Drivers(), "pgx") {
sql.Register("pgx", pgxDriver)
}
sql.Register("pgx/v5", pgxDriver)
// extra stuff
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the other PR I refer to the driver directly as pgx, if you want to see that option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I see. So by removing lib/pq we lose the postgres plugin, meaning we have ensure it's registered if we want the sql.Open("postgres", "<conn>") to work.
I've done some reading on this and it seems that even the lib/pq maintainers suggest using the pgx driver 🤔
For users that require new features or reliable resolution of reported bugs, we recommend using pgx which is under active development.
Here's what I think we should do:
- Register
pgxas normal, and add an additonalpostgres_v2driver to our SQL components, which acts as an alias to the underlyingpgxdriver.driver: postgres => lib/pq::postgresdriver: postgres_v2 => github.com/jackc/pgx/v4/stdlib::pgx
input:
sql_select:
driver: postgres_v2 # will use pgx- We can then add an integration test using this
pgxdriver (aliased in config bypostgres_v2) in ourintegration_test.go.
bento/internal/impl/sql/integration_test.go
Line 731 in e93b7b1
| func TestIntegrationPostgres(t *testing.T) { |
-
Then, in the next release, we can change the default
postgresto point topgxand deprecatelib/pqsupport:driver: postgres_legacy => lib/pq::postgresdriver: postgres => github.com/jackc/pgx/v4/stdlib::pgxdriver: postgres_v2 => github.com/jackc/pgx/v4/stdlib::pgx
Note: specifying this
driver: postgres_v2should probably log a warning saying it's being removed in next minor release, else we just keep it around if it's not too much extra complexity.
| }) | ||
| } | ||
|
|
||
| func getDriver(conf *service.ParsedConfig, log *service.Logger) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's log this in sqlOpenWithReworks rather? Then we wont need to use the getDriver in each component
bento/internal/impl/sql/conn_fields.go
Line 366 in 6640566
| func sqlOpenWithReworks(ctx context.Context, logger *service.Logger, driver, dsn string, connSettings *connSettings) (*sql.DB, error) { |
| @@ -0,0 +1,18 @@ | |||
| package postgrespgx | |||
|
|
|||
| import ( | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not register pgx as a driver in public?
bento/public/components/sql/package.go
Lines 11 to 12 in ecb62f3
| // Import all (supported) sql drivers. | |
| _ "github.com/ClickHouse/clickhouse-go/v2" |
| var warnPostgresV2Once sync.Once | ||
|
|
||
| func warnIfPostgresV2(driver string, log *service.Logger) { | ||
| if driver != "postgres_v2" || log == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add postgres_v2 as a driver in the conn_fields? It should be in
bento/internal/impl/sql/conn_fields.go
Lines 19 to 20 in 6640566
| var driverField = service.NewStringEnumField("driver", "mysql", "postgres", "clickhouse", "mssql", "sqlite", "oracle", "snowflake", "trino", "gocosmos", "spanner"). | |
| Description("A database [driver](#drivers) to use.") |
Draws on this issue (which I ran into myself), in which
sql_insertdoesn't actually support Postgres array columns. I was left trying to manually escape convoluted strings using regex, and handcraft array literals, which is really brittle.This is a different approach to the other PR, in which I add pgx as an alternate driver. In this case, as discussed here, I'm replacing lib/pq directly, and adding a shim that retains support for referencing the new postgres driver with
postgres. Given thatpgx/v4was already used here, I stuck with that, though obviously you may choose to upgrade tov5.It doesn't redefine any settings otherwise, just reuses everything from the original postgres driver.
I've run
go testoninternal/impl/sqland built a docker image, and everything seems to be fully working, at least for my somewhat narrow use case (that in the issue referenced above).That being said, I would like to state for the record that this was LLM-assisted, though obviously the change is pretty straightforward. I figure it doesn't hurt to mention in case I've missed something obvious.