Skip to content
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

Upgrade jackc/pgx to v5 #819

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Upgrade jackc/pgx to v5 #819

wants to merge 5 commits into from

Conversation

alnr
Copy link
Contributor

@alnr alnr commented Mar 31, 2023

What is being done in this PR?

Closes #818

Supersedes #814

What are the main choices made to get to this solution?

jackc/pgx/v5 has slightly different handling of SQL NULLs than v4. Otherwise, this was a drop-in replacement.

List the manual test cases you've covered before sending this PR:

Ran tests against postgres locally.

aeneasr and others added 3 commits March 13, 2023 18:37
Version 5 of pgx finally resolves some long-awaited issues when PostgreSQL shuts down.

The upgrade is a drop-in replacement with the only change being the pgconn import path.

See ory/hydra#3432

Signed-off-by: aeneasr <3372410+aeneasr@users.noreply.github.com>
In order to support improve PostgreSQL connectivity, Go 1.18 is required. Go 1.17's support ended 7 months ago.

Signed-off-by: aeneasr <3372410+aeneasr@users.noreply.github.com>
Copy link
Member

@sio4 sio4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a comment on Float which could be applied to the others too. Otherwise, it looks good to me!
Thanks!

@@ -20,6 +20,9 @@ func (f Float) Interface() interface{} {
func (f *Float) Scan(src interface{}) error {
var str string
switch t := src.(type) {
case nil:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be changed to the general nil checking like the String case for readability? The other types too, including Map.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, that change broke the whole thing haha. We must distinguish between a nil interface and a non-nil interface backed by nil.

I've changed the String case to match the other implementations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe a different confusion here. if the type switch's case nil works like your PR, that means the src has no specific type and is actually nil so src == nil should be true. If the src is a "non-nil interface with a nil value", the type of src can't be nil and the case nil will not work. What is the actual src in here? something like *[]string(nil)?

I hope we have enough description on the PR or linked issue so we can have a solid reference for the chance in the code history.

@sio4 sio4 self-assigned this Apr 3, 2023
@sio4 sio4 added s: accepted This proposal was accepted. Someone can start working on it. enhancement New feature or request labels Apr 3, 2023
@alnr alnr marked this pull request as draft April 4, 2023 13:38
@alnr
Copy link
Contributor Author

alnr commented Apr 4, 2023

pgx/v5 is a pretty big change from v4. I've changed this PR back to draft while waiting for some insights here: jackc/pgx#1566

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request s: accepted This proposal was accepted. Someone can start working on it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature: upgrade jackc/pgx to v5
3 participants