Feedback-ID: i0fe9450f:Fastmail
Date: Fri, 7 Mar 2025 08:15:38 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Jacob Champion <jacob.champion@enterprisedb.com>
Cc: Andres Freund <andres@anarazel.de>, Robert Haas <robertmhaas@gmail.com>,
	Noah Misch <noah@leadboat.com>,
	Andrew Dunstan <andrew@dunslane.net>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
	Euler Taveira <euler.taveira@enterprisedb.com>,
	Daniel Gustafsson <daniel@yesql.se>
Subject: Re: [PATCH] pg_stat_activity: make slow/hanging authentication more
 visible
Message-ID: <Z8osml5sp7Q3LIhZ@paquier.xyz>
References: <CAOYmi+kA2VNne4AofaMv-C7vYSmdcAXaFWh3W4Nw11c0Rc+QgA@mail.gmail.com>
 <CAOYmi+=tkATXFmvvAzwLTwq52YrezFopeoPrkW3pXv6LDFYT6A@mail.gmail.com>
 <Z8fW2PcLkkXaK9L8@paquier.xyz>
 <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>
 <Z8kfmvKOfYA91sNE@paquier.xyz>
 <CAOYmi+nbgAifBRHy6_dSFGcwtRk78YTbVTO=tb+1HwqM0gwbCw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="GuyiGmbs0+HRQNeK"
Content-Disposition: inline
In-Reply-To: <CAOYmi+nbgAifBRHy6_dSFGcwtRk78YTbVTO=tb+1HwqM0gwbCw@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Z8osml5sp7Q3LIhZ%40paquier.xyz>
Precedence: bulk


--GuyiGmbs0+HRQNeK
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Mar 06, 2025 at 02:25:07PM -0800, Jacob Champion wrote:
> On Wed, Mar 5, 2025 at 8:08=E2=80=AFPM Michael Paquier <michael@paquier.x=
yz> wrote:
>> +  WHERE state =3D 'starting' and wait_event =3D 'init-pre-auth';});
>=20
> Did you have thoughts on expanding the check to backend_type [1]?
>=20
>> + # Give up.  The output of the last attempt is logged by query(),
>> + # so no need to do anything here.
>> + return 0;
>=20
> One of my primary complaints about the poll_query_until()
> implementation is that "giving up" in this case means continuing to
> run pieces of the test that have no business running after a timeout,
> and increasing the log noise after a failure. I'm not sure how loudly
> to complain in this particular case, since I know we use it
> elsewhere...

Indeed.  The existing poll_query_until() is a bit more reliable in
terms of error handling, even with a very low PG_TEST_TIMEOUT_DEFAULT.

A second thing that was bugging me on a second lookup this morning is
how we should handle error cases.  A background psql process depends
on what the caller defines for ON_ERROR_STOP.  In the case of this
test, we're OK to fail immediately because we expect the queries to
always work.  I'm not sure if this is fine by default, especially if
callers of this routine expect to have the same properties as
poll_query_until() in Cluster.pm.  They would not, because a
BackgroundPsql is an entire different object, except if given options
when staring psql to act like that.

I have applied the simplest patch for now, to silence the failures in
the CI, and included your suggestion to add a check on the
backend_type for the extra safety it offers.

I'd like the addition of the poll_query_until() in the long-term, but
I'm really not sure if the semantics would be right this way under a
background psql.  In the auth 007 test, they would be OK, but it could
be surprising if we have other callers that begin relying on it.
--
Michael

--GuyiGmbs0+HRQNeK
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmfKLJoACgkQnvQgOdby
QH1PRw/8CRgRYf7c2qxNkv2ol6egJ7HWhzZG+/ZNGyHAWMuu5g5MS32xWS04Mwx6
Ae0QsKEhAesXvv3nU/opNh4NvEnsIpXSTock4NHkxrZB6qvFSvObmNT3pNdA6G8S
lRw5XgPytB6e38wjWtI93nFIWMHbll+ysE04R0ZUXx1I9xUGApl8Q1Fwsvw0Fvos
XwhYR0AhtC+fKLIM9BKcHFxeZx4inh6nh5IgEL/nitjga5+bE7ML5lXOtgsVOodx
cZo39/VAF/vR6F9TEjb5XXcsIZHklCFueUqNLHxZTHuoLKlmkcpYCcVX93l70J8D
5X6rPY6QxVed6JWGVhmd7+PRLmWaK6+qO8Xb8eJ3GD/vygUozQBGSOj1I5WdWHz5
QV4t/2bapaiB2e6i3dtZfs00saccxMyZI5XFxYBn+IQ1HhdbcXYIaI/Lzq22UQWz
OpZpONa3MNyZK91cIX3aEC4zZGu8UzCgg0f+Q42OyqeprQlbdUyyUnuwJDKUax0U
rVu6HhaW/11ZmGBq+sKCHWc9Tz6aJF7pJr2PpFtLZkspnnGFnyGspBl4DiaBG1Og
PF0CmXbd+/qKTkWxKy5V6IB22v5s3C07tTE4A917Seae7CCiHM5kR2VZWlERlHnK
9vtME9aEqmylD79ZFfvE2VyrCjf5KyfFVxTbsG7t01VY3avgWXc=
=L9Wo
-----END PGP SIGNATURE-----

--GuyiGmbs0+HRQNeK--