In short, all the 4 patches look good to me. Thanks for picking this up!
On 06/03/2025 22:16, Andres Freund wrote:
On 2025-03-05 20:49:33 -0800, Noah Misch wrote:
This behaviour makes it really hard to debug problems. It'd have been a lot
easier to understand the problem if we'd seen psql's stderr before the test
died.
I guess that mean at the very least we'd need to put an eval {} around the
->pump() call., print $self->{stdout}, ->{stderr} and reraise an error?
That sounds right.
In the attached patch I did that for wait_connect(). I did verify that it
works by implementing the wait_connect() fix before fixing
002_connection_limits.pl, which fails if a sleep(1) is added just before the
proc_exit(1) for FATAL.
+1. For the archives sake, I just want to clarify that this pump stuff
is all about getting better error messages on a test failure. It doesn't
help with the original issue.
This is all annoyingly complicated, but getting good error messages is
worth it.
On 2025-03-05 08:23:32 +0900, Michael Paquier wrote:>> Why not adding an injection point with a WARNING or a LOG generated,
then
check the server logs for the code path taken based on the elog() generated
with the point name?
I think the log_min_messages approach is a lot simpler. If we need something
like this more widely we can reconsider injection points...
+1. It's a little annoying to depend on a detail like the "client
backend process exited" debug message, but seems like the best fix for now.
I also attached a patch to improve connect_fails()/connect_ok() test names a
bit. They weren't symmetric and I felt they were lacking in detail for the
psql return code check.
+1.
While we're at it, attached are a few more cleanups I noticed.
Another annoying and also funny problem I saw is this failure:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2025-03-06%2009%3A18%3A21
2025-03-06 10:42:02.552 UTC [372451][postmaster][:0] LOG: 1800 s is outside the valid
range for parameter "authentication_timeout" (1 s .. 600 s)
I had to increase PG_TEST_TIMEOUT_DEFAULT due to some other test timing out
when run under valgrind (due to having to insert a lot of rows). But then this
test runs into the above issue.
The easiest way seems to be to just limit PG_TEST_TIMEOUT_DEFAULT in this
test.
LGTM
--
Heikki Linnakangas
Neon (https://neon.tech)
From a4871adb5de6f363b96e9a2d5723c32330ad1e6e Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Thu, 6 Mar 2025 22:45:55 +0200
Subject: [PATCH 1/1] Fix test name and username used in failed connection
attempt
The first failed connection tests the "regular" connections limit, not
the reserved limit.
The username doesn't really matter, but since the previous successful
connections used "regress_reserved", it seems weird to switch back to
"regress_regular" for the expected-to-fail attempt.
---
src/test/postmaster/t/002_connection_limits.pl | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/test/postmaster/t/002_connection_limits.pl b/src/test/postmaster/t/002_connection_limits.pl
index 8cfa6e0ced5..94c087a2751 100644
--- a/src/test/postmaster/t/002_connection_limits.pl
+++ b/src/test/postmaster/t/002_connection_limits.pl
@@ -53,7 +53,7 @@ push(@sessions, background_psql_as_user('regress_regular'));
push(@sessions, background_psql_as_user('regress_regular'));
$node->connect_fails(
"dbname=postgres user=regress_regular",
- "reserved_connections limit",
+ "regular connections limit",
expected_stderr =>
qr/FATAL: remaining connection slots are reserved for roles with privileges of the "pg_use_reserved_connections" role/
);
@@ -61,7 +61,7 @@ $node->connect_fails(
push(@sessions, background_psql_as_user('regress_reserved'));
push(@sessions, background_psql_as_user('regress_reserved'));
$node->connect_fails(
- "dbname=postgres user=regress_regular",
+ "dbname=postgres user=regress_reserved",
"reserved_connections limit",
expected_stderr =>
qr/FATAL: remaining connection slots are reserved for roles with the SUPERUSER attribute/
--
2.39.5
