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

Multiple hosts support fix #1470 #1711

Closed

Conversation

denchistyakov
Copy link

No description provided.

@denchistyakov
Copy link
Author

denchistyakov commented Aug 29, 2018

Fix #1470

@charmander
Copy link
Collaborator

Looks like this is missing implementation for the pure JS driver? Also, a second module for connection string parsing that isn’t compatible with libpq connection strings AIUI doesn’t seem right vs. adding to pg-connection-string. Tests are necessary as well.

@denchistyakov
Copy link
Author

Looks like this is missing implementation for the pure JS driver?

You want to ask me for porting this logic in JS:

When multiple hosts are specified, or when a single hostname is translated to multiple addresses, all the hosts and addresses will be tried in order, until one succeeds. If none of the hosts can be reached, the connection fails. If a connection is established successfully, but authentication fails, the remaining hosts in the list are not tried.

?

Also, a second module for connection string parsing that isn’t compatible with libpq connection strings

In what kind of cases? It's work with all types of documented URI except for unix sockets, for them I use previous module.
Work of pg-connection-string is based on using of url.parse and it's doen't work with case of multiple hosts in uri.

@vitaly-t
Copy link
Contributor

vitaly-t commented Sep 8, 2018

It works with all types of documented URI except for unix sockets

@denchistyakov why doesn't it work with the sockets?

The standard for unix sockets inside a connection string is to have the server address ending with .sock, which works perfectly well with connection-string.

Here's example:

const parse = require('connection-string');
const cn = parse('postgres://user:[email protected]/myDB');
console.log(cn);

outputs:

ConnectionString {
  protocol: 'postgres',
  user: 'user',
  password: 'pass',
  hosts: [ { name: 'server-address.sock', isIPv6: false } ],
  path: [ 'myDB' ] }

This means your check for the use of a socket would be as simple as:

const isSocket = cn.hosts && cn.hosts[0].name && cn.hosts[0].name.endsWith('.sock');

And when your socket address contains /, they are expected to be encoded as %2F. That's the standard, as the server address must be URL-encoded.

So everything should work fine for unix sockets. You do not need to use pg-connection-string for that.

@charmander
Copy link
Collaborator

In what kind of cases? It's work with all types of documented URI except for unix sockets, for them I use previous module.

For example: when there are spaces in some part. libpq handles them fine. I’m sure there are many other differences (since MongoDB and libpq are unrelated), but that’s an obvious one because tests had to be removed.

@vitaly-t
Copy link
Contributor

vitaly-t commented Sep 8, 2018

when there are spaces in some part

MongoDB spec for connections strings is the most sensible one. And it requires that the server address is URL-encoded, which means spaces must be passed in as %20.

This generic approach is much safer than all the custom/proprietary ones used so far by this driver. It should finally shift towards use of the generic connection string, IMO.

@charmander
Copy link
Collaborator

MongoDB spec for connections strings is the most sensible one.

For a PostgreSQL driver? No, I think libpq’s implementation is the most sensible compatibility target.

@jimlindeman
Copy link

Is there anything blocking this functionality from being merged at this time? Is it the missing tests for this capability?

A project I'm working on needs this functionality to use multiple HA-proxy layers in front of the postgres DB's, so we 'HA' the HA-proxies.

@charmander
Copy link
Collaborator

Is there anything blocking this functionality from being merged at this time?

@jimlindeman Yes, it’s missing the implementation.

@radoslawroszkowiak
Copy link

@charmander, which implementation do you mean? PG native?
Why not to allow multiple hosts when on libpq, and i.e. ignore any but first host and throw some warning when native in use?

@vitaly-t
Copy link
Contributor

@radoslawroszkowiak Libq does support the feature natively, b.t.w., so it should be done for both at the same time, IMO.

@charmander
Copy link
Collaborator

@radoslawroszkowiak I mean the non-pg-native implementation. The JS driver.

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