public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: Andres Freund <[email protected]>
Cc: Jacob Champion <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Noah Misch <[email protected]>
Cc: Andrew Dunstan <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Euler Taveira <[email protected]>
Cc: Daniel Gustafsson <[email protected]>
Subject: Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
Date: Thu, 6 Mar 2025 13:07:54 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <lfzorjb3rnywykl4dazkk24h3dlqulufwmvknj3hby2tc6hwh2@vprzh3stdr6h>
References: <[email protected]>
<soexrl7oeyku24bj3czupxmv27ow35u6edymp5y3oyoysbe2kb@r3tgoos2xp2x>
<CAOYmi+kA2VNne4AofaMv-C7vYSmdcAXaFWh3W4Nw11c0Rc+QgA@mail.gmail.com>
<CAOYmi+=tkATXFmvvAzwLTwq52YrezFopeoPrkW3pXv6LDFYT6A@mail.gmail.com>
<[email protected]>
<CAOYmi+=+py0pJSKCvZSXZbA9CRxc=OtLXx90BRDj-Rdx2f2WDg@mail.gmail.com>
<CAOYmi+nxNCQcTQE-tQ7Lwpe4cYc1u-yxwEe5kt2AVN+DXXVVbQ@mail.gmail.com>
<tfc4r4sr5m47f42nwsymadafjr3h7sjskilq62s64ujdpi4oxc@dfgxzqcxjrjy>
<CAOYmi+n0EU-O5fRR9SmtekLp4XZt80OieMYJapfy=Z9zTkG6rQ@mail.gmail.com>
<lfzorjb3rnywykl4dazkk24h3dlqulufwmvknj3hby2tc6hwh2@vprzh3stdr6h>
On Wed, Mar 05, 2025 at 08:55:55PM -0500, Andres Freund wrote:
> Once we've slept for 10+ seconds without reaching the target, sleeping for
> 100us is way too short a sleep and just wastes CPU cycles. A decent portion
> of the CPU time when running under valgrind is wasted just due to way too
> tight retry loops.
>
> That's harder to do if we have many places polling.
>
> But anyway, I digress, that's really not related to your change.
Please let me agree with your previous argument, then. While looking
at the test when reviewing the patch a couple of days ago, I was also
wondering why we could not have a poll_query_until() in BackgroundPsql
and gave up on the idea.
Honestly, I don't see a reason not to introduce that, like in the
attached. BackgroundPsql->query() does all the job already, and it is
possible to rely on $PostgreSQL::Test::Utils::timeout_default in the
loops, so that's simple, and it makes the test a bit easier to parse.
--
Michael
Attachments:
[text/x-diff] v2-0001-Fix-race-condition-in-pre-auth-test.patch (3.9K, ../[email protected]/2-v2-0001-Fix-race-condition-in-pre-auth-test.patch)
download | inline diff:
From 8caea1d434aa1cbd6f1da777b89dd4895a88b44f Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Thu, 6 Mar 2025 13:06:05 +0900
Subject: [PATCH v2] Fix race condition in pre-auth test
---
src/test/authentication/t/007_pre_auth.pl | 36 ++++++---------
.../perl/PostgreSQL/Test/BackgroundPsql.pm | 44 +++++++++++++++++++
2 files changed, 57 insertions(+), 23 deletions(-)
diff --git a/src/test/authentication/t/007_pre_auth.pl b/src/test/authentication/t/007_pre_auth.pl
index a638226dbaf1..fb4241cac4ab 100644
--- a/src/test/authentication/t/007_pre_auth.pl
+++ b/src/test/authentication/t/007_pre_auth.pl
@@ -43,36 +43,26 @@ $psql->query_safe("SELECT injection_points_attach('init-pre-auth', 'wait')");
# authentication. Use the $psql connection handle for server interaction.
my $conn = $node->background_psql('postgres', wait => 0);
-# Wait for the connection to show up.
-my $pid;
-while (1)
-{
- $pid = $psql->query(
- "SELECT pid FROM pg_stat_activity WHERE state = 'starting';");
- last if $pid ne "";
+# Wait for the connection to show up in pg_stat_activity, with the wait_event
+# of the injection point.
+my $res = $psql->poll_query_until(
+ qq{SELECT count(pid) > 0 FROM pg_stat_activity
+ WHERE state = 'starting' and wait_event = 'init-pre-auth';});
+ok($res, 'authenticating connections are recorded in pg_stat_activity');
- usleep(100_000);
-}
-
-note "backend $pid is authenticating";
-ok(1, 'authenticating connections are recorded in pg_stat_activity');
+# Get the PID of the backend waiting, for the next checks.
+my $pid = $psql->query(
+ qq{SELECT pid FROM pg_stat_activity
+ WHERE state = 'starting' and wait_event = 'init-pre-auth';});
# Detach the waitpoint and wait for the connection to complete.
$psql->query_safe("SELECT injection_points_wakeup('init-pre-auth');");
$conn->wait_connect();
# Make sure the pgstat entry is updated eventually.
-while (1)
-{
- my $state =
- $psql->query("SELECT state FROM pg_stat_activity WHERE pid = $pid;");
- last if $state eq "idle";
-
- note "state for backend $pid is '$state'; waiting for 'idle'...";
- usleep(100_000);
-}
-
-ok(1, 'authenticated connections reach idle state in pg_stat_activity');
+$res = $psql->poll_query_until(
+ qq{SELECT state FROM pg_stat_activity WHERE pid = $pid;}, 'idle');
+ok($res, 'authenticated connections reach idle state in pg_stat_activity');
$psql->query_safe("SELECT injection_points_detach('init-pre-auth');");
$psql->quit();
diff --git a/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm b/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm
index c611a61cf4e6..83ecf8b3e720 100644
--- a/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm
+++ b/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm
@@ -61,6 +61,7 @@ use Config;
use IPC::Run;
use PostgreSQL::Test::Utils qw(pump_until);
use Test::More;
+use Time::HiRes qw(usleep);
=pod
@@ -172,6 +173,49 @@ sub wait_connect
die "psql startup timed out" if $self->{timeout}->is_expired;
}
+
+=pod
+
+=item $session->poll_query_until($query [, $expected ])
+
+Run B<$query> repeatedly, until it returns the B<$expected> result
+('t', or SQL boolean true, by default).
+Continues polling if B<query> returns an error result.
+
+Times out after $PostgreSQL::Test::Utils::timeout_default seconds.
+
+Returns 1 if successful, 0 if timed out.
+
+=cut
+
+sub poll_query_until
+{
+ my ($self, $query, $expected) = @_;
+
+ $expected = 't' unless defined($expected); # default value
+
+ my $max_attempts = 10 * $PostgreSQL::Test::Utils::timeout_default;
+ my $attempts = 0;
+
+ while ($attempts < $max_attempts)
+ {
+ my $ret = $self->query($query);
+
+ if ($ret eq $expected)
+ {
+ return 1;
+ }
+
+ # Wait 0.1 second before retrying.
+ usleep(100_000);
+ $attempts++;
+ }
+
+ # Give up. The output of the last attempt is logged by query(),
+ # so no need to do anything here.
+ return 0;
+}
+
=pod
=item $session->quit
--
2.47.2
[application/pgp-signature] signature.asc (833B, ../[email protected]/3-signature.asc)
download
view thread (24+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox