Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 10 Jan 2024 15:40:36 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Jelte Fennema-Nio <postgres@jeltef.nl>
Cc: Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com>,
	Anton Kirilov <antonvkirilov@gmail.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Robert Haas <robertmhaas@gmail.com>, pgsql-hackers@postgresql.org
Subject: Re: Add PQsendSyncMessage() to libpq
Message-ID: <ZZ475K5dDxDlUl97@paquier.xyz>
References: <202311081620.s2a7ao5jtlpe@alvherre.pgsql>
 <b878b8d2-8481-4fde-a374-5ccb9168d4e4@gmail.com>
 <CAO6_Xqo_H3-ykt_Hmip-TKNBVWZbRthLvFu4qTpQSSEgV4dWmA@mail.gmail.com>
 <CAGECzQQqkkvSko4fZUBAmVMd=VmC4nXdyOBO=s_42HzKJ=GaEA@mail.gmail.com>
 <ZZC3ywNby6FtSagL@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="VtOlmxVeT9DqXv53"
Content-Disposition: inline
In-Reply-To: <ZZC3ywNby6FtSagL@paquier.xyz>
Archived-At: <https://www.postgresql.org/message-id/ZZ475K5dDxDlUl97%40paquier.xyz>
Precedence: bulk


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

On Sun, Dec 31, 2023 at 09:37:31AM +0900, Michael Paquier wrote:
> On Fri, Dec 29, 2023 at 12:52:30PM +0100, Jelte Fennema-Nio wrote:
>> Those are some nice improvements. And I think once this patch is in,
>> it would make sense to add the pgbench feature you're suggesting.
>> Because indeed that makes it see what perf improvements can be gained
>> for your workload.
>=20
> Yeah, that sounds like a good idea seen from here.  (Still need to
> look at the core patch.)

 PQpipelineSync(PGconn *conn)
+{
+	return PQsendPipelineSync(conn) && pqFlush(conn) >=3D 0;
+}
[...]
+	 * Give the data a push if we're past the size threshold. In nonblock
+	 * mode, don't complain if we're unable to send it all; the caller is
+	 * expected to execute PQflush() at some point anyway.
 	 */
-	if (PQflush(conn) < 0)
+	if (pqPipelineFlush(conn) < 0)
 		goto sendFailed;

I was looking at this patch, and calling PQpipelineSync() would now
cause two calls of PQflush() to be issued when the output buffer
threshold has been reached.  Could that lead to regressions?

A second thing I find disturbing is that pqAppendCmdQueueEntry() would
be called before the final pqFlush(), which could cause the commands
to be listed in a queue even if the flush fails when calling
PQpipelineSync().

Hence, as a whole, wouldn't it be more consistent if the new
PQsendPipelineSync() and the existing PQpipelineSync() call an
internal static routine (PQPipelineSyncInternal?) that can switch
between both modes?  Let's just make the extra argument a boolean.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmWeO+QACgkQnvQgOdby
QH0P0hAAm287xtErkDzwfEZXIlOQmIqSVr6OeYxUPEQZaJ/MFopTghA1h/8d8VSV
lZkRsFFNtcmpK2InK/dxaFYDAiPLDdL+XEYjD/704fXBYv7467rFWyM1TsY41zpt
A7v09s7NAlrX9PIk6ON8vEB6HPZ6+IQVzPFxbzqz3/RF7D9JH7hordHsoSWzewqK
rSEJmFB6FXP9Oook1ktboAifDkiB3DjKm5Dhcm69QTTL1wEW+9CsppPbl/VTxjCN
+u9JyxCveWWvx+05QcTdqwzNA3eX0ScKwGb6yDu93zalAXd8a/FlRetrfT7STtOz
oQaff2uINGLR6ks3ZTbHXS4AlUdJUWVJmoPVTtxclxOPP1OBtBcbNI5iYsHChM6D
pIVDElyLougr/ESNVnmRasZHPw2aEbF2wihlVCNFsjWJgXOO4T7hPXvBFgOtDdHH
Ody6hEXjg3zdVICy0eTHq2jYu4qU5O8aALh005IKr+vf3zMnXAxriK3UifaWI3Ac
NqiM33yJg4O2OthiQkmzD4tPNBlowUCIgv0mAkUemnagNhTHrTqXByGG7enSEHPO
oEUPZWyO8PtuSdF4WUOfWGgRoid7KYZVvuGNwW76vNojtHjZjNXd8kRKn8x7+g4p
/zHwIaUj3Bk8B64jSva6oThiQWWI7zByRKxu4ajBn2ly5mpUGxo=
=zyYL
-----END PGP SIGNATURE-----

--VtOlmxVeT9DqXv53--