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

feat: allow connection URI and add additional config parameters #841

Merged
merged 1 commit into from
Mar 24, 2022
Merged

feat: allow connection URI and add additional config parameters #841

merged 1 commit into from
Mar 24, 2022

Conversation

iwpnd
Copy link
Member

@iwpnd iwpnd commented Mar 20, 2022

Hi 👋

Now with the new pgx it is easier to pass a connection string which was on the wishlist for some issues (eg. #774). If the uri is set in the config.toml said uri is used without compromise as long as the basic information user, password, host, port and database are set. The sslmode is added if not specifically set.

Something on my personal wish list was the addition of "max_connection_idle_time" and "max_connection_lifetime" to the config that default to the pgx default if not specifically set.

@iwpnd iwpnd requested review from gdey and ARolek as code owners March 20, 2022 15:45
@iwpnd
Copy link
Member Author

iwpnd commented Mar 20, 2022

woops. url.Values.Has() was only added with 1.17 - will fix

@coveralls
Copy link

coveralls commented Mar 20, 2022

Pull Request Test Coverage Report for Build c579f3eb8-PR-841

  • 113 of 125 (90.4%) changed or added relevant lines in 2 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.4%) to 45.53%

Changes Missing Coverage Covered Lines Changed/Added Lines %
provider/postgis/error.go 6 11 54.55%
provider/postgis/postgis.go 107 114 93.86%
Files with Coverage Reduction New Missed Lines %
provider/postgis/postgis.go 2 61.91%
Totals Coverage Status
Change from base Build 905eed645: 0.4%
Covered Lines: 5577
Relevant Lines: 12249

💛 - Coveralls

provider/postgis/error.go Outdated Show resolved Hide resolved
provider/postgis/error.go Outdated Show resolved Hide resolved
provider/postgis/postgis.go Outdated Show resolved Hide resolved
provider/postgis/postgis.go Outdated Show resolved Hide resolved
provider/postgis/postgis.go Outdated Show resolved Hide resolved
provider/postgis/postgis.go Outdated Show resolved Hide resolved
provider/postgis/postgis.go Outdated Show resolved Hide resolved
provider/postgis/postgis.go Outdated Show resolved Hide resolved
provider/postgis/postgis.go Outdated Show resolved Hide resolved
@iwpnd
Copy link
Member Author

iwpnd commented Mar 20, 2022

Thank you very much for the detailed review, @gdey. I’ll read up on error handling some more and fix the issues. 🙏🏻

Copy link
Member

@ARolek ARolek left a comment

Choose a reason for hiding this comment

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

Overall this LGTM! I think we should deprecate the old config strategy in favor of the connection URI string. We should do this over 2 releases, so we should add a warning during the parsing step now.

provider/postgis/postgis.go Outdated Show resolved Hide resolved
provider/postgis/postgis.go Show resolved Hide resolved
Copy link
Member

@gdey gdey left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@ARolek ARolek left a comment

Choose a reason for hiding this comment

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

One minot comment, but LGTM! Thank you for the contribution!

provider/postgis/README.md Outdated Show resolved Hide resolved
feat: allow connection URI and add additional config parameters

fix: dependencies

fix: dont return empty objects

fix: errors

fix: return if both Msg and Err are empty

chore: add deprecation warning, set DefaultSSLMode to prefer

chore: add TODO reminder to remove BuildURI when host/port connection is deprecated

docs: update with connection string

docs: add a comment on top to state the context the file is used within

docs: fix versions
@ARolek ARolek merged commit 75498b9 into go-spatial:v0.15.x Mar 24, 2022
@durkie
Copy link

durkie commented Mar 15, 2023

is there an example way to connect to a local postgresql database via UNIX socket connection with the new uri= scheme? I'm trying loads of different permutations but keep getting errors about missing user credentials or missing port.

@iwpnd
Copy link
Member Author

iwpnd commented Mar 15, 2023

I think you have to escape it or use host as Param. Never tried it myself tho.

postgresql:///dbname?host=/var/lib/postgresql

postgresql://%2Fvar%2Flib%2Fpostgresql/dbname

@durkie
Copy link

durkie commented Mar 15, 2023

Yeah I haven't had much luck with either of those formats: escaping it with %2F results in parse errors, and not escaping it says "missing port". Adding a "port" via something like /var/lib/postgresql:5432 or ?port=5432 doesn't seem to work

@iwpnd
Copy link
Member Author

iwpnd commented Mar 15, 2023

Im sorry, can't help right now. Tegola is using pgx under the hood, maybe dig a little if that is an issue with pgx.

@durkie
Copy link

durkie commented Mar 15, 2023

Sure thing. After digging around a bit, it looks like the main issue is using url.Parse to validate the postgresql connection string, and it chokes on finding a unix pathname. But that was helpful to dig around, and I found something that works:

uri = "postgresql://tegola:xxxx@127.0.0.1:5432/db_name?host=/run/postgresql&sslmode=disable"

  • you have to provide both a valid URL and then the socket as a host= parameter.
  • provides a dummy URL to connect to: the port 5432 in 127.0.0.1:5432 is important, because it uses this to discover the unix socket at /run/postgresql/.s.PGSQL.5432. The address doesn't seem to matter.
  • sslmode=disable is necessary because otherwise you'll get TLS termination errors

@ARolek
Copy link
Member

ARolek commented Mar 27, 2023

@durkie thanks for digging into this. Would be great to get some better error handling on these connection params. I would happily accept a PR if you feel so inclined. ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants