chore/fix(sql): remove lib/pq in favor of pgx

[maxtheaxe]

Draws on this issue (which I ran into myself), in which sql_insert doesn'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 that pgx/v4 was already used here, I stuck with that, though obviously you may choose to upgrade to v5.

It doesn't redefine any settings otherwise, just reuses everything from the original postgres driver.

---

I've run go test on internal/impl/sql and 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.

[gregfurman]

Why do we need this driver approach? Can we not just directly swap out lib/pq usage with "github.com/jackc/pgx/v4/stdlib"?

[maxtheaxe]

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
}

[maxtheaxe]

In the other PR I refer to the driver directly as pgx, if you want to see that option.

[gregfurman]

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 🤔

See lib/pq/README.md#status:

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:

1. Register pgx as normal, and add an additonal postgres_v2 driver to our SQL components, which acts as an alias to the underlying pgx driver.
   • driver: postgres => lib/pq::postgres
   • driver: postgres_v2 => github.com/jackc/pgx/v4/stdlib::pgx

input:
  sql_select:
    driver: postgres_v2 # will use pgx

3. We can then add an integration test using this pgx driver (aliased in config by postgres_v2) in our integration_test.go.

bento/internal/impl/sql/integration_test.go

Line 731 in e93b7b1

func TestIntegrationPostgres(t *testing.T) {

4. Then, in the next release, we can change the default postgres to point to pgx and deprecate lib/pq support:

   • driver: postgres_legacy => lib/pq::postgres
   • driver: postgres => github.com/jackc/pgx/v4/stdlib::pgx
   • driver: postgres_v2 => github.com/jackc/pgx/v4/stdlib::pgx

   Note: specifying this driver: postgres_v2 should 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.

[gregfurman]

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) {

[gregfurman]

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"

[gregfurman]

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.")

[gregfurman]

Hey @maxtheaxe 👋 Do you still have capacity for this? Otherwise happy for myself (or @jem-davies) to push it over-the-line.

Lmk!

[maxtheaxe]

Hey @gregfurman. Sorry for the delay here—been busy. I will try to find time this weekend. If I can't, it's all yours. Apologies for the trouble.

[gregfurman]

@maxtheaxe No worries at all! This isn't a blocker for anyone atm, so I just wanted to make sure that you're still keen to finish it off.

TBH I think the library change you proposed is a super neat addition and would love to see it actualised 🙂

[maxtheaxe]

I'm definitely still game, it just isn't critical to what I'm doing at work. We're launching very shortly, so I'll do it on my own time. I appreciate you following up here to check in!

[maxtheaxe]

Hey @gregfurman. Sadly, I think I'm probably letting either pride or selfishness get in the way of a good feature here. I still don't have time, but if it becomes a blocker, please don't hesitate to finish without me. If all else fails, eventually I will get to it.

To be clear, I'm not saying I don't still want to do it, I'm just recognizing how long it's dragged on at this point.

[gregfurman]

@maxtheaxe Thanks for letting me know! I'll place this back into draft and you can pick it back up when you're ready 🙂