Fix #9134 - Update pgx to v4 #9182
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,9 +10,8 @@ import ( | |
| "strings" | ||
| "time" | ||
|
|
||
| "github.com/jackc/pgx/v4" | ||
| "github.com/jackc/pgx/v4/stdlib" | ||
|
|
||
| "github.com/influxdata/telegraf" | ||
| "github.com/influxdata/telegraf/config" | ||
|
|
@@ -90,7 +89,7 @@ func parseURL(uri string) (string, error) { | |
| // packages. | ||
| type Service struct { | ||
| Address string | ||
| OutputAddress string | ||
| MaxIdle int | ||
| MaxOpen int | ||
| MaxLifetime config.Duration | ||
|
|
@@ -111,33 +110,16 @@ func (p *Service) Start(telegraf.Accumulator) (err error) { | |
| // Specific support to make it work with PgBouncer too | ||
| // See https://github.com/influxdata/telegraf/issues/3253#issuecomment-357505343 | ||
| if p.IsPgBouncer { | ||
|
Comment on lines
-122
to
-133
There was a problem hiding this comment. it looks like we're still losing support for these data types? Do you know that they're both covered by the new v4 library? There was a problem hiding this comment. The mapping of the data types is done inside the driver. Within pgx v4 the mapping has been outsourced and there is a separate repository for it. The version of the driver I am using, uses 1.3.0 of the outsourced types repositories. Furthermore, I have also made the corresponding integration test functional and could already test the corresponding extraction of the metrics there. There was a problem hiding this comment. got it. just trying to make sure we didn't inadvertently remove support for these two types. Otherwise we're good to go There was a problem hiding this comment. Thanks for the link to the pgtype mapping. I didn't know that's how it works but it makes sense now. |
||
|
Comment on lines
-114
to
-137
There was a problem hiding this comment. It does look like this block is important to pgbouncer support. Does pgx v4 handle this in another way or does this PR remove pgbouncer support? There was a problem hiding this comment. In my view, the configuration of the driver should be automatic and the manual setup is no longer needed in v4. Furthermore, the PR should only fix a bug and not remove a feature. To eliminate this risk I will first try to create an integration test. |
||
| // Remove DriveConfig and revert it by the ParseConfig method | ||
| // See https://github.com/influxdata/telegraf/issues/9134 | ||
| d, err := pgx.ParseConfig(p.Address) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| d.PreferSimpleProtocol = true | ||
|
|
||
| connectionString = stdlib.RegisterConnConfig(d) | ||
| } | ||
|
|
||
| if p.DB, err = sql.Open("pgx", connectionString); err != nil { | ||
|
|
@@ -166,8 +148,8 @@ func (p *Service) SanitizedAddress() (sanitizedAddress string, err error) { | |
| canonicalizedAddress string | ||
| ) | ||
|
|
||
| if p.OutputAddress != "" { | ||
| return p.OutputAddress, nil | ||
| } | ||
|
|
||
| if strings.HasPrefix(p.Address, "postgres://") || strings.HasPrefix(p.Address, "postgresql://") { | ||
|
|
||
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.
Do we no longer need any driver config?
The comment on 111 points to a comment around this.
Also this thread seems to suggest we need/needed to register data types?
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 my opinion, the configuration of the driver is no longer needed inside v4. However, I'm not 100% sure and I don't know enough about PgBouncer. For this reason, I will first try to create an integration test and clear the corresponding doubts based on this test.
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.
That's great thanks