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

Improve warning when connection to unix socket fails #837

Merged

Conversation

JelteF
Copy link
Member

@JelteF JelteF commented Apr 25, 2023

PgBouncer is hard to debug when it's trying to connect to Postgres
server over a UNIX socket but that connection fails. You would get an
unhelpful error message like this:

WARNING sbuf_connect failed: No such file or directory

This change starts including the path in the error message:

WARNING sbuf_connect failed to connect to UNIX socket /aaaaa/.s.PGSQL.5432: No such file or directory

It also includes the address+port for IPv4/IPv6 connection attempts.

Fixes #835

@JelteF JelteF mentioned this pull request Apr 25, 2023
@JelteF JelteF force-pushed the improve-warning-on-unix-connect-failure branch from f7c7fa1 to 2fcf827 Compare April 25, 2023 13:28
@eulerto
Copy link
Member

eulerto commented Apr 25, 2023

Why don't you provide messages for IPv4 and IPv6? AFAICS, this log_warning call does not provide a PgSocket as first parameter so address information (postgres/euler@unix:9915) provided by log_socket_prefix is not added after the prefix (2023-02-27 16:27:24.299 -03 [33171] DEBUG). It is not possible to provide PgSocket to sbuf API (it is in a different layer), hence, you should construct a string that contains host + port or Unix socket path for sbuf. Perhaps, add this address information before the message (to be similar to what logging_prefix_cb does).

@JelteF
Copy link
Member Author

JelteF commented Apr 25, 2023

I wasn't able to trigger the WARNING for IPv4/IPv6 sockets. They would error during reading instead of erroring at connect. Those errors during reading would become a LOG with the log_socket_prefix. So those are already pretty helpful.

@eulerto
Copy link
Member

eulerto commented Apr 25, 2023

I didn't try to reproduce it but the following code can trigger it. Looking at man socket, I expect some errors (such as EACCESS, EMFILE, ENFILE, ENOMEM) in certain situations.

    sock = socket(sa->sa_family, SOCK_STREAM, 0);
    if (sock < 0) { 
        /* probably fd limit */
        goto failed;
    }

@JelteF JelteF force-pushed the improve-warning-on-unix-connect-failure branch from 2fcf827 to 67b0983 Compare April 26, 2023 13:26
@JelteF
Copy link
Member Author

JelteF commented Apr 26, 2023

Thanks for the pointer. By manually setting my file limit very low I was able to trigger the WARNING for IPv4/IPv6 sockets too. So now the patch includes a better error for those as well.

@JelteF JelteF requested a review from eulerto April 26, 2023 13:28
@eulerto
Copy link
Member

eulerto commented May 1, 2023

Since sa2str encapsulates the print-a-network-address logic, you don't need the if/else. Use only the last else statements.

PgBouncer is hard to debug when it's trying to connect to Postgres
server over a UNIX socket but that connection fails. You would get an
unhelpful error message like this:
```
WARNING sbuf_connect failed: No such file or directory
```

This change starts including the path in the error message:
```
WARNING sbuf_connect failed to connect to UNIX socket /aaaaa/.s.PGSQL.5432: No such file or directory
```

It also includes the address+port for IPv4/IPv6 connection attempts.
@JelteF JelteF force-pushed the improve-warning-on-unix-connect-failure branch from 67b0983 to 05dd56f Compare May 1, 2023 21:22
@JelteF
Copy link
Member Author

JelteF commented May 1, 2023

Makes total sense. Pushed that now. I had a bit of a brainfart I think because I misremembered the fix for this issue: libusual/libusual@9469419

Copy link
Member

@eulerto eulerto left a comment

Choose a reason for hiding this comment

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

LGTM

@JelteF JelteF merged commit 1cd6e5c into pgbouncer:master May 2, 2023
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.

Debian and Unix Sockets
2 participants