-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Fix #9134 - Update pgx to v4 #9182
Conversation
- Update go.mod - Update go.sum - Add license information - Update check-deps.sh script - Update documentation
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
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.
Makes sense to me
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.
Thanks for the PR! Overall looks good just a few questions in the code.
Were you able to run the integration tests? As we don't currently run them as part of our circleci checks.
RuntimeParams: map[string]string{ | ||
"client_encoding": "UTF8", | ||
}, | ||
CustomConnInfo: func(c *pgx.Conn) (*pgtype.ConnInfo, 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.
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
I executed the appropriate integration tests locally and looking especially at the tests where I made changes to the actual plugins. |
d := &stdlib.DriverConfig{ | ||
ConnConfig: pgx.ConnConfig{ | ||
PreferSimpleProtocol: true, | ||
RuntimeParams: map[string]string{ | ||
"client_encoding": "UTF8", | ||
}, | ||
CustomConnInfo: func(c *pgx.Conn) (*pgtype.ConnInfo, error) { | ||
info := c.ConnInfo.DeepCopy() | ||
info.RegisterDataType(pgtype.DataType{ | ||
Value: &pgtype.OIDValue{}, | ||
Name: "int8OID", | ||
OID: pgtype.Int8OID, | ||
}) | ||
// Newer versions of pgbouncer need this defined. See the discussion here: | ||
// https://github.com/jackc/pgx/issues/649 | ||
info.RegisterDataType(pgtype.DataType{ | ||
Value: &pgtype.OIDValue{}, | ||
Name: "numericOID", | ||
OID: pgtype.NumericOID, | ||
}) | ||
|
||
return info, 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Hi @helenosheaa, I have looked at the PGBouncer integration test and run it in the main and feature branches. Here I could see the same error message in both branches. I get the following error message and the test is normally skipped: Container: Console:
Furthermore, I can also see a corresponding error message in both scenarios in the container log. I am not a PGBouncer expert and would like to know if there is anything I need to consider when setting up the containers (PostgreSQL + PGBouncer) and running the tests? When was the last time the tests were run? Anyway, I then looked at the PgBouncer integration test and the configuration of the connection within service.go and decided to use the PgBouncer integration test. I subsequently modified the container used accordingly and published it to Dockerhub. Furthermore, I modified the service.go according to the issue linked here. PgBouncer does not know the configuration parameter Afterwards I modified the integration test of PgBouncer, because there seems to be an update in PgBouncer and now other metrics are returned in a modified structure. Summary |
…date the pgbouncer connection configuration
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
scripts/check-deps.sh
Outdated
# dependency is replaced in go.mod | ||
github.com/satori/go.uuid) continue;; | ||
# dependency is replaced in go.mod | ||
github.com/satori/go.uuid) continue;; |
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 you restore the original spacing here? Thanks!
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.
Thanks for the hint. It's done.
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
@ZPascal thanks for working on the integration test! I don't think it was in a working state previously so I really appreciate it. What was the reason for changing to your own docker image? I saw that you added |
@helenosheaa Gladly, no problem
I had seen that the existing image was no longer regularly maintained and updated. Furthermore, the stats_users parameter must be set in the corresponding configuration. For this reason, I decided to create an updated image.
Right. It's necessary to set the parameter |
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
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.
Ok, thanks for clarifying!
@reimda are you also happy for this to be merged now there's a working integration test?
@helenosheaa Any news ? |
info.RegisterDataType(pgtype.DataType{ | ||
Value: &pgtype.OIDValue{}, | ||
Name: "int8OID", | ||
OID: pgtype.Int8OID, | ||
}) | ||
// Newer versions of pgbouncer need this defined. See the discussion here: | ||
// https://github.com/jackc/pgx/issues/649 | ||
info.RegisterDataType(pgtype.DataType{ | ||
Value: &pgtype.OIDValue{}, | ||
Name: "numericOID", | ||
OID: pgtype.NumericOID, | ||
}) |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the link to the pgtype mapping. I didn't know that's how it works but it makes sense now.
Required for all PRs:
This change fixes a bug inside the pgx library. Until now, it was unfortunately not possible to validate a CA certificate in verify-ca mode. The update of the library to V4 solved the corresponding problem. More details can be found, in the bug linked below.
resolves #9134
I updated pgx to version v4.6.0, fixed a typo, adapted the corresponding documentation and the PgBouncer functionality inside the
service.go
file to the new library version.@helenosheaa Could you please, review this change?