Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 12 Sep 2024 08:42:25 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Jacob Champion <jacob.champion@enterprisedb.com>
Cc: Andrew Dunstan <andrew@dunslane.net>, Noah Misch <noah@leadboat.com>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
	Euler Taveira <euler.taveira@enterprisedb.com>,
	Robert Haas <robertmhaas@gmail.com>
Subject: Re: [PATCH] pg_stat_activity: make slow/hanging authentication more
 visible
Message-ID: <ZuIq4TOOHmkFE5Al@paquier.xyz>
References: <CAOYmi+=60deN20WDyCoHCiecgivJxr=98s7s7-C8SkXwrCfHXg@mail.gmail.com>
 <20240630174812.d1.nmisch@google.com>
 <CAOYmi+=1Y4HKATADR=WYOMhxCkRQpoxH2rY0t1An6A5su1Cw-w@mail.gmail.com>
 <34cc0828-d988-4503-9944-ea6288f0ca8a@dunslane.net>
 <ZtUCcpf9y7gnHifu@paquier.xyz>
 <CAOYmi+n4RhhoaduZ-ndZGR_=kkzUjnaZtzi8g2GFsqt==Hhopg@mail.gmail.com>
 <Zt_ZVcW4NmPM9NRB@paquier.xyz>
 <CAOYmi+k_ZHg94iVKtfbBGU2y8brox7rpMVY9QU1PzB7c6RaTLQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="VuvlzXfpkkr5T7C8"
Content-Disposition: inline
In-Reply-To: <CAOYmi+k_ZHg94iVKtfbBGU2y8brox7rpMVY9QU1PzB7c6RaTLQ@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ZuIq4TOOHmkFE5Al%40paquier.xyz>
Precedence: bulk


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

On Wed, Sep 11, 2024 at 02:29:49PM -0700, Jacob Champion wrote:
> On Mon, Sep 9, 2024 at 10:30=E2=80=AFPM Michael Paquier <michael@paquier.=
xyz> wrote:
>> No.  My question was about splitting pgstat_bestart() and
>> pgstat_bestart_pre_auth() in a cleaner way, because authenticated
>> connections finish by calling both, meaning that we do twice the same
>> setup for backend entries depending on the authentication path taken.
>> That seems like a waste.
>=20
> I can try to separate them out. I'm a little wary of messing with the
> CRITICAL_SECTION guarantees, though. I thought the idea was that you
> filled in the entire struct to prevent tearing. (If I've misunderstood
> that, please let me know :D)

Hm, yeah.  We surely should be careful about the consequences of that.
Setting up twice the structure as the patch proposes is kind of
a weird concept, but it feels to me that we should split that and set
the fields in the pre-auth step and ignore the irrelevant ones, then
complete the rest in a second step.  We are going to do that anyway if
we want to be able to have backend entries earlier in the
authentication phase.

>> Couldn't it be better to have a one-one mapping
>> instead, adding twelve entries in wait_event_names.txt?
>=20
> (I have no strong opinions on this myself, but while the debate is
> ongoing, I'll work on a version of the patch with more detailed wait
> events. It's easy to collapse them again if that gets the most votes.)

Thanks.  Robert is arguing upthread about more granularity, which is
also what I understand is the original intention of the wait events.
Noah has a different view.  Let's see where it goes but I've given my
opinion.

> I can test for specific contents of the entry, if you'd like. My
> primary goal was to test that an entry shows up if that part of the
> code hangs. I think a regression would otherwise go completely
> unnoticed.

Perhaps that would be useful, not sure.  Based on my first
impressions, I'd tend to say no to these extra test cycles, but I'm
okay to be proved wrong, as well.
--
Michael

--VuvlzXfpkkr5T7C8
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmbiKuEACgkQnvQgOdby
QH18nQ/8DVSXU3MlphkEzpwZMiZckMbdmWXG8t0xt7aoOypazXOj4GmCyP9R37Gi
sbX6AODsGkPhh6vEItzOQDbtRR2L6MW8Mssy6weTOT5204bVR2l+0/U7uj6adzAC
ik/37WcydYGN7qBoTJZb8XAoPaYSsSXgEp1N6ZPak9ej7iMEPDs7/kdI+DSfDmxa
xce68k/sD9IoNYiY+QX6U6iA4qdtYNaedNL7whZNY1CyBz11JNEGA9tSdV6m5xea
IZdSL6R6ah4a/r8GGhv8anh7Lm/FEcamLWnAmiDMkFjbsMHUdQVQzHVTAEZvImMb
n7twONZnecVQs927JDY3VTlo/XDSnhPB9PhOk/SyiVIDZvQVE9LrjvR7nBW2nxhA
oE724iz4e+0H1NGBNB45aRVx9Aa7ticllrM9BkckEO6htl/jQISqrMBZe+kSAyZL
5D11N+YOFwzh7er0+zI+WoOptWNxnQFrnU7xRL07KJNgs4pM42Fw5wHprfCVVGAa
RqSIWhyXgd4m09WJnHeVhFC+tmu9R9wxuHM0MGQpWsiBuAIkTDslFwkOya+H7/0J
Iqc+SD9S6muNPJ4lrfDKupRWiA00VZdk1QgSWQ53MyeY/YehZzCNcpAAVvrX1DFa
GT0iX0RSdE00BoWS0l3wCk9CUfi5Mm9vl+IouXhJmGkCpEHNawk=
=Tvfs
-----END PGP SIGNATURE-----

--VuvlzXfpkkr5T7C8--