Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 27 Jun 2024 14:39:21 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Jelte Fennema-Nio <postgres@jeltef.nl>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Nathan Bossart <nathandbossart@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: libpq: Fix lots of discrepancies in PQtrace
Message-ID: <Znz7CSktKAVbm4Ck@paquier.xyz>
References: <ZnxBpzWyqTf3aV5d@nathan>
 <202406261636.w7emb6xa6eto@alvherre.pgsql>
 <CAGECzQTJ44hh4UWBKuvmeC7VVGt3vvcG8rKE3YER=aODXxR_JA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="hyJe8P7VXOs8QepH"
Content-Disposition: inline
In-Reply-To: 
 <CAGECzQTJ44hh4UWBKuvmeC7VVGt3vvcG8rKE3YER=aODXxR_JA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/Znz7CSktKAVbm4Ck%40paquier.xyz>
Precedence: bulk


--hyJe8P7VXOs8QepH
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Jun 26, 2024 at 10:02:08PM +0200, Jelte Fennema-Nio wrote:
> On Wed, 26 Jun 2024 at 18:36, Alvaro Herrera <alvherre@alvh.no-ip.org> wr=
ote:
> > Thanks, Nathan.  I'm holding myself responsible for the rest ... will
> > handle soon after the branch.
>=20
> Sounds great. Out of curiosity, what is the backpatching policy for
> something like this? Honestly most of these patches could be
> considered bugfixes in PQtrace, so backpatching might make sense. OTOH
> I don't think PQtrace is used very much in practice, so backpatching
> might carry more risk than it's worth.

0001 getting on HEAD after the feature freeze as a cleanup piece
cleanup is no big deal.  That's cosmetic, still OK.

Looking at the whole, the rest of the patch set qualifies as a new
feature, even if they're aimed at closing existing gaps.
Particularly, you have bits of new infrastructure introduced in libpq
like the current_auth_response business in 0004, making it a new
feature by structure.

+	conn->current_auth_response =3D AUTH_RESP_PASSWORD;
 	ret =3D pqPacketSend(conn, PqMsg_PasswordMessage, pwd_to_send, strlen(pwd=
_to_send) + 1);
+	conn->current_auth_response =3D AUTH_RESP_NONE;

It's a surprising approach.  Future callers of pqPacketSend() and
pqPutMsgEnd() would easily miss that this flag should be set, as much
as reset.  Isn't that something that should be added in input of these
functions?

+	AuthResponseType current_auth_response;
I'd recommend to document what this flag is here for, with a comment.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmZ8+wkACgkQnvQgOdby
QH1VZw/9G74zkK8/rJvK7xMZsgbPKNwwnf9dTKPhgtKamC16BSstLmrsdf8yLMwC
FLh46zOKN77hvEpmQd9/YJKduUtUJyfjh7reQUZIVYbH56pf1YESo41eCVw+F3br
5zuQZfK6J6pN1RFrEv7Xagxd8rYdzGM7ElvJy5ifLRl0dXwgCWDYPkpUtrD/6MXQ
CDaQYUC4/2glG74ojGfeBDsRyeRcfytidjpwByWhvFqxaAtFQ94x+UhenwfJijNB
nlJyqVsgSiDotORu2sBHq9aEZQifo7OgamlDW8IbHapBkyyq5g8WeqOrfKcjpG0T
Xkq+oRpYghcYzuyGkkWghEvXhK5+C5WQlCcTlMqb5i7ROCyuw0DeuWwnZIZfUo59
n15OveB75bmmu05kx8MWFIPuSrhz5UKCCxy022yjQnfAz2t6USI71AX4OrXguPtE
wg4HH+Gu533sz+9K3Wv+VTxX+0LOdscRQJwpO4ESJ3g+aGZMy/Zu5j5b2wdNmmGI
nC9dnxk7G3AZyzKvwhNJE5ROxoXWuJMmfHNPuYkPHUPRtVHmYBafesXxEMjf0Vh8
GhoAIYdWpyZ5ZkIk9KU50bu+DzXBYGif5mArV7Nbacc4p9CjME1O5MM8nhhA1kc3
ZmJPUzNUKCifPAX96nrYpwVQBp3Uj0Hpgsb5S5eYwTpj75pryW0=
=yts9
-----END PGP SIGNATURE-----

--hyJe8P7VXOs8QepH--