Skip to content

Attempt to fix inconsistent error#1152

Merged
JelteF merged 15 commits intopgbouncer:masterfrom
AndrewJackson2020:inconsistent_error_message
Dec 2, 2024
Merged

Attempt to fix inconsistent error#1152
JelteF merged 15 commits intopgbouncer:masterfrom
AndrewJackson2020:inconsistent_error_message

Conversation

@AndrewJackson2020
Copy link
Copy Markdown
Contributor

@AndrewJackson2020 AndrewJackson2020 commented Aug 29, 2024

This changes server_login_retry to also cache the error message. This makes it much easier to debug an authentication problem between PgBouncer and Postgres, because now the client always sees the message, not just the first time. Before you'd have to check the PgBouncer logs to find the actual error message.

Fixes #1149

@JelteF
Copy link
Copy Markdown
Member

JelteF commented Sep 22, 2024

I think we'd still want the fast fail logic, to be able to quickly surface an error to all waiting clients. But we'd want want that error to contain the original error message too, in addition to the error about the fast failure being triggered.

@AndrewJackson2020
Copy link
Copy Markdown
Contributor Author

Yeah I think that makes sense. Will figure out how to make that happen.

@AndrewJackson2020
Copy link
Copy Markdown
Contributor Author

AndrewJackson2020 commented Sep 23, 2024

Just pushed a new commit that restores the fast fail logic but saves the previous error message to present to clients. Let me know if this is more in the right direction.

Also not sure why this is failing on freebsd and one of the Linux test sets. Will look into this.

Copy link
Copy Markdown
Member

@JelteF JelteF left a comment

Choose a reason for hiding this comment

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

I think this is good overall. For the FreeBSD failure, feel free to just get it to pass by skipping the test. The error reporting on FreeBSD to the client is known to be bad/broken in the test suite.

AndrewJackson2020 and others added 4 commits October 7, 2024 17:17
Co-authored-by: Jelte Fennema-Nio <github-tech@jeltef.nl>
Co-authored-by: Jelte Fennema-Nio <github-tech@jeltef.nl>
@AndrewJackson2020
Copy link
Copy Markdown
Contributor Author

I think this is good overall. For the FreeBSD failure, feel free to just get it to pass by skipping the test. The error reporting on FreeBSD to the client is known to be bad/broken in the test suite.

Changed the test so that it skips FreeBSD. I'm still seeing FreeBSD fail with the message ld-elf.so.1: /lib/libc.so.7: version FBSD_1.8 required by /usr/local/libexec/cabal/pandoc not found on this PR, #1138, and #1137.

@eulerto
Copy link
Copy Markdown
Member

eulerto commented Oct 9, 2024

Indeed. I asked Cirrus Labs about the FreeBSD image and it seems we are using an EOL image. I created ande merged the PR #1183 to fix it.

Revert the skip and fix the other tests.

@AndrewJackson2020
Copy link
Copy Markdown
Contributor Author

Indeed. I asked Cirrus Labs about the FreeBSD image and it seems we are using an EOL image. I created ande merged the PR #1183 to fix it.

Revert the skip and fix the other tests.

Can confirm that the image update fixed #1138 and #1137. Still seeing some flakiness on some of the test runs in this PR though.

@AndrewJackson2020
Copy link
Copy Markdown
Contributor Author

@JelteF I believe that I was able to get the tests to work on test/test_auth.py::{test_change_server_password_reconnect,test_change_server_password_server_lifetime} by adding a sleep statement at the end. Possibly that test needed more time in order for the final error message to hit the log before the log_contains could register it. Either that or the test is flaky for some reason.

Either way let me know if this needs more work or it looks good to you.

@JelteF JelteF merged commit 2b6e9cf into pgbouncer:master Dec 2, 2024
JelteF added a commit to JelteF/pgbouncer that referenced this pull request Dec 8, 2024
The new error message test introduced by pgbouncer#1152 is failing on Windows.
This slipped past CI, because Windows CI was broken completely
before pgbouncer#1216.

For now this disables the test on Windows to make CI green, but it might
be nice to actually fix the behaviour.
JelteF added a commit that referenced this pull request Dec 8, 2024
The new error message test introduced by #1152 is failing on Windows.
This slipped past CI, because Windows CI was broken completely
before #1216.

For now this disables the test on Windows to make CI green, but it might
be nice to actually fix the behaviour.
@JelteF JelteF mentioned this pull request Jan 10, 2025
2 tasks
rajaryanece pushed a commit to rajaryanece/pgbouncer that referenced this pull request Jan 16, 2025
This changes `server_login_retry` to also cache the error message. This
makes it much easier to debug an authentication problem between
PgBouncer and Postgres, because now the client always sees the message,
not just the first time. Before you'd have to check the PgBouncer logs
to find the actual error message.

Fixes pgbouncer#1149
rajaryanece pushed a commit to rajaryanece/pgbouncer that referenced this pull request Jan 16, 2025
The new error message test introduced by pgbouncer#1152 is failing on Windows.
This slipped past CI, because Windows CI was broken completely
before pgbouncer#1216.

For now this disables the test on Windows to make CI green, but it might
be nice to actually fix the behaviour.
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.

Inconsistent Error Message

3 participants