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

Add multihost connstring support #714

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

costela
Copy link

@costela costela commented Feb 12, 2018

This PR is a first attempt at supporting the newer multi-host features of libpq 10.
It adds support for providing multiple hosts in the connstring (either URL or key=value form) and also for the target_session_attrs connection parameter.

It includes a few noteworthy changes, which could use some further scrutiny:

  1. expanded the travis setup script to also set up a secondary node in replication mode.
  2. dropped testing for unsupported postgres versions. This was needed to keep the primary/secondary testing setup simple, since older versions lack pg_basebackup. This decision can be revisited, if testing for older versions is still deemed necessary (or the patch can be split into a separate PR)
  3. analogously to libpq, we loop over the different hosts to attempt connections. We accumulate errors and return a new errorSlice, which also implements the error interface. To keep backwards compatibility for people expecting specific error types, a special case for len(hosts)==1 was added.
  4. for each connection attempt, if the client requested it, we check if the server is in read-write mode. For this, we also follow libpq's example and execute a simpleQuery during connection establishment, checking the output of SHOW transaction_read_only.
  5. lastly, the TestOpenURL test was changed to not start a transaction, instead executing a SHOW server_version. This is needed for testing connections to the secondary node.

PTAL

fixes #683

@costela
Copy link
Author

costela commented Feb 12, 2018

PS.: yay! travis build number 1000! 🍾 🎉

@aaerofeev
Copy link

aaerofeev commented Jun 26, 2018

Hi! When this pull request will be in production? Or works was stop?

@alexanderGalushka
Copy link

Yeah, I have the same question. Is it going to production anytime soon?

@costela
Copy link
Author

costela commented Jul 17, 2018

I have to rebase and fix the small conflict introduced by further work on master. Unfortunately I'm currently on paternity leave and probably won't get around to it in the next few weeks.

After that someone with merge access still has to take a look and see if this PR is an acceptable approach at all. Since there was zero feedback up until this point, I'm not sure how long this will take.
It would be nice if anyone else with go skills would take a look at the code and start reviewing. Maybe this would get the ball rolling.

@kneal
Copy link

kneal commented Oct 26, 2018

Is this issue still being worked on?

@bpicolo
Copy link

bpicolo commented Nov 21, 2018

@mjibson Is this something that could be prioritized? This is a critical feature for HA usage without deploying complicated HA solutions. The go-mysql equivalent is ahead of the postgres curve here.

@maddyblue
Copy link
Collaborator

I'm unlikely to review this soon, sorry. You'll have to maintain your own fork for now.

@jnicholls
Copy link

@mjibson Can you offload the review to another contributor then? This feature should not be stonewalled.

@costela Are you able to rebase and revisit this work? If not I am happy to take it over, but will need to open my own PR unless you give me access to write to your fork.

@maddyblue
Copy link
Collaborator

I'm not aware of any reviewers who are active on this project (including myself), so there's really no one to hand it off to.

@bpicolo
Copy link

bpicolo commented Dec 18, 2018

The primary golang postgres connection library has zero active maintainers? That's...wild. Have you considered reaching out to Golang mailing groups for assistance in maintaining the lib, or similar?

I don't want to suggest that you personally should have the responsibility for upkeep here, to be sure, but it's pretty astounding to me that nobody has picked this up.

I'd be open to helping with some housekeeping, but can't say I currently have a particularly deep domain expertise here.

@maddyblue
Copy link
Collaborator

maddyblue commented Dec 18, 2018

It is indeed sad this has no active maintainers. Open source has its problems.

@jnicholls
Copy link

Agreed this is the nature of the beast, but a part of that responsibility is to hand over the reigns when one cannot self-maintain any further. I assume you are prepared to do that. I'm a willing and able participant. I'll talk to @fdr to see if he can give me access, I work closely with the Citus Data folks.

@maddyblue
Copy link
Collaborator

Sure. I don't have permissions to grant permissions so I'm not able to do anything. A few of us from cockroach just asked the last time this was a problem and they gave us commit here. We just keep limping along.

@jnicholls
Copy link

Roger that. I also wonder if most people's alternative has been to go with and contribute to pgx.

@maddyblue
Copy link
Collaborator

I use pgx for all of my new work, so yes. We should all do that.

@jnicholls
Copy link

I'm down with that.

@costela
Copy link
Author

costela commented Dec 19, 2018

AFAICT neither go-pg nor pgx currently support the equivalent of this PR, though I see @bpicolo already suggested a similar approach here jackc/pgx#299 and there's at least initial prodding here go-pg/pg#999.

@jnicholls if by any chance you decide to come back to this PR, let me know and I'll give you access to my fork. Would gladly take the help! 👍

@kneal
Copy link

kneal commented Dec 19, 2018

To reiterate are you guys essentially saying this isn't going to get merged because lack of contributors to the project? And if that is the case how will the upstream projects using the library know that the project is slowly dying?

Reason, I ask is because I was interested in this feature because I use Vault with the Postgres backend and we excited when we found this PR. (We as in my team that manages the Vault instances)

@bpicolo
Copy link

bpicolo commented Dec 19, 2018

And if that's the case, should/can we get a warning into the Readme? This lib is by far the most google-able postgres lib for go (and definitely has the most Github stars)

@hannesdejager
Copy link

hannesdejager commented Apr 11, 2019

I've now gone ahead and do this: https://github.com/hannesdejager/pgdfailwrap since I could also not find a go solution to this. Perhaps its usable for some but it really only caters for my use-case.

@fdr
Copy link
Member

fdr commented Apr 11, 2019

For what it's worth, I think pgx is fine software.

@germm
Copy link

germm commented May 20, 2020

Is there a chance that this pull request will get merged soon? Is there anything we can do to help with this pull request?

@knadh
Copy link

knadh commented Dec 24, 2020

This looks like a simple enough patch. Would the maintainers be willing to merge this given someone can review and test it and share a report?

@fdr
Copy link
Member

fdr commented Dec 24, 2020

@knadh hey, thanks for pinging. @lib/pq-committers, anyone up for this one? It seems overdue. Maybe I would do it myself, but I don't have time to react to any consequences/defects.

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.

add support for multiple hosts in connection settings (for feature parity with libpq)