-
Notifications
You must be signed in to change notification settings - Fork 82
Handle connection string for unix sockets correctly #305
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
Conversation
2b6262a to
6c51751
Compare
|
I suppose it shouldn't be too hard to keep a fall back for the old behaviour to avoid breaking? |
|
Ok that is a simple enough solution for backwards compatibility, I will implement it soon |
|
Thanks for the review and the helpful suggestions. I added the check for backwards compatibility by removing the socket name suffix if it is found. I decided against adding a test for it because I think it should be considered just a fix and deprecated behavior (if you prefer otherwise I can add it). I changed the check for detecting when to use a unix socket to this: if Path.new(@conninfo.host).absolute?So that the check also work on windows when a drive letter is found, matching the libpq docs
On windows the SOCKET_SEARCH paths do nothing as they are based on unix, to fix this we could detect the OS and search different paths, but I'm not sure what paths to put there. |
|
Thanks! I just updated the CI to run with newer versions of crystal and the current set of supported Postgres versions. Could you rebase on that, and then also add a line to the changelog for this? |
|
Ok great! I'll add a line to the changelog. I don't understand what should I do, to which branch should I rebase? |
When connecting to a unix socket the path should be interpreted as the path to the directory that contains the socket rather that the path to the socket itself.
|
There is just one new commit on master c50de77 that I put there an hour or so ago. There is a button I can push here on this PR that can do it, I'll push it now but just know there'll be a commit on your branch you didn't do. That can sometimes cause annoying git problems if the other person isn't aware it happened. |
|
Thanks! |
Ah ok no problem, I didn't notice your new commit |
When using connection strings with unix sockets the path should be interpreted as the path to the directory that contains the unix socket rather than the path to socket itself as per spec here:
https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNECT-HOST
The port also can be specified in the URI parameters, as per spec:
https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNECT-PORT
The name of the socket is based on the configured port in PostgreSQL this PR also handles this case by generating the socket file name based on the specified port.
https://www.postgresql.org/docs/current/runtime-config-connection.html#GUC-UNIX-SOCKET-DIRECTORIES
This change would be breaking for current users that rely on current behavior, I can fix this by supporting both possibilities, but this would add complexity and support an out of spec behavior.
Fixes: #297