feat: add option to enable Redshift OIDs#1291
feat: add option to enable Redshift OIDs#1291niger-prequel wants to merge 6 commits intolib:masterfrom
Conversation
|
Isn't that It's not really clear to me what exactly this fixes; did you get an error? Some unexpected behaviour? Something else? Having a small test case and the output of it being run with PQGO_DEBUG=1 would be helpful. |
|
Looking at this code a bit more, your new code maps e.g. So it seems it's probably just some specific type that's missing? It could be oid package just needs updating: it was last updated for PostreSQL 9.4. |
|
@arp242 its not identical. I updated the map to just be the OIDs that Redshift has that postgres doesn't.
The hard part is you can't run Redshift locally. you need an instance in the cloud to repro what the OIDs will be. I got them by running the following in 2 of our different Redshift instances to confirm they are stable across instances. SELECT oid, typname
FROM pg_type WHERE oid < 7000 order by oid desc; |
|
Okay, so Redshift has a few extra types. None of these oids are currently used, I think we can just add that to oid/types.go? If there is a conflict in the future then we can cross that bridge when we get there, which we may never do – oids are not added very often: once in 2026 and 2025, zero in 2024 and 2023, 2022, two in 2021, etc. Possibly we could only lookup these oids based on the version string Redshift sends, or some other signal that identifies it as such (if any). I would like to avoid adding connection parameters for this sort of thing. |
|
Updated oid package with: #1292 I think all that's needed with that is: And that should be enough? |
|
Actually, never mind – there are a few duplicates. I should have actually tried it before posting 😅 So it does need a separate map, which is okay. But I'd really like to avoid adding a connection string. It already gets the PostgreSQL version in conn.processParameterStatus(); what is that for Redshift? You can see all the startup parameters with something like: Or anything else that connects; PQGO_DEBUG=1 should work from applications too. |
|
@arp242 happy to make it dynamic. I used |
|
So I haven't looked at any of this code at all since I started maintaining this. Looking at this a bit more, I think this entire strategy of generating a list might be incorrect. What it should do instead if query For example custom types will fail too: func TestXXX(t *testing.T) {
db := pqtest.MustDB(t)
pqtest.Exec(t, db, `create type bug_status as enum ('new', 'open', 'closed')`)
pqtest.Exec(t, db, `create temp table bugs (id serial, status bug_status)`)
pqtest.Exec(t, db, `insert into bugs (status) values ($1), ($2)`, "new", "open")
rows, err := db.Query(`select * from bugs`)
if err != nil {
t.Fatal(err)
}
defer rows.Close()
coltypes, err := rows.ColumnTypes()
if err != nil {
t.Fatal(err)
}
for _, c := range coltypes {
fmt.Printf("%s (%s)\t", c.Name(), c.DatabaseTypeName())
}
fmt.Println()
for rows.Next() {
var (
id int
status string
)
err := rows.Scan(&id, &status)
if err != nil {
t.Fatal(err)
}
fmt.Printf("%v\t%v\n", id, status)
}
}Outputs: No type for the status column, as that's a custom enum type. If we query pg_type, then it should always be correct: for any PostgreSQL version, for any Redshift, cockroachdb, etc. etc. Need to think about it; I want to hold off merging new things for a week or two anyway, because I did a release today and would rather not merge anything so it's easier to do a new bugfix release for regressions (if needed). Also have some other projects I want to work on. |
|
@arp242 appreciate you taking time to think on this. Some things to consider about your proposed rewrite:
|
|
It should load and cache on use, rather than loading all of them up-front. I think that's also what e.g. libpq or pgx do. It already does the same with with timezone names. I think what I'll do is close my "update to PostgreSQL 18.3" PR, merge this (in 2-3 weeks as mentioned) as it solves a practical problem, and create an issue for improving this in the future. AFAIK no one else has reported any problems with this, there's more pressing matters to work on, and this PR won't create any problems for further improvements down the line. |
2 participants
Add option for Redshift-specific OIDs mapping
Description
This PR introduces support for Redshift-specific OID mapping within the
lib/pqdriver, enabled via a newredshift_oidsconnection string parameter.Currently, there is no dedicated, robust, and
database/sql-compatible driver for Amazon Redshift in the Go ecosystem. Because Redshift is heavily based on a fork of PostgreSQL 8.0.2, many developers rely onlib/pqto connect to Redshift databases. While this works well for standard system types, Redshift introduces several custom data types (such as_spectrum_array,_spectrum_map, etc.) with Object Identifiers (OIDs) not found in Postgres.When these types are queried through
Rows.ColumnTypes(), they return an emptyrows.DatabaseTypeName()string sincelib/pqdoes not natively recognize them. By giving developers a way to map these Redshift-specific OIDs to their actual type names, we can drastically improve the experience for developers usinglib/pqas a Redshift driver.Changes
redshift_oidsparameter that can be toggled via the standard DSN configuration.redshiftTypeNamedictionary mapping Redshift's unique OIDs to their human-readable type names.rows.ColumnTypeDatabaseTypeName()to fall back to the custom Redshift OID mapping if the column's type string evaluates to empty (unrecognized by standard Postgres OIDs) and theredshift_oidsfeature is explicitly enabled.Safe by Default
To guarantee that this feature introduces zero regressions for existing PostgreSQL users, this OID mapping sits strictly behind an explicit config boolean (
redshift_oids). Unless explicitly enabled in the connection string (redshift_oids=1), the driver operates exactly as it did before.Furthermore, because Redshift inherited its system types directly from PostgreSQL 8.0.2, there are no overlapping OIDs assigned to radically different types. As an extra layer of caution, the Redshift mapping map acts purely as a fallback mechanism; it takes effect only if standard Postgres natively evaluates the system type's name to
"".