Feedback-ID: i0fe9450f:Fastmail
Date: Tue, 18 Feb 2025 16:22:53 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com>
Cc: Jelte Fennema-Nio <postgres@jeltef.nl>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Add Pipelining support in psql
Message-ID: <Z7Q1TTtOxYEiNn0h@paquier.xyz>
References: <CAGECzQTjCEXpJGOq9O6Nf+p4zRwjt-b1p6kACWVNJ+uYcLGvyg@mail.gmail.com>
 <Z0gQ-DYex9dbsBWe@paquier.xyz>
 <CAGECzQTiCvOxxuA98Ei+qVb1px82eP-iRvJvnvrcKU1de55-Pw@mail.gmail.com>
 <CAO6_XqrseE1UGxMEHu3CARdn5HW_C4rqR3Xy=Vg2sfHjPRa1og@mail.gmail.com>
 <CAO6_Xqr10ncZPmVzn1gUT2nHu8Bd+u3SkbgF4Uw9TL8FUA5fbQ@mail.gmail.com>
 <CAO6_XqrDPpaHqvKA0r8kfuHBE8BZ5ZqWLwucu30cXobeGtvBMA@mail.gmail.com>
 <CAGECzQQfjzxhYvH9LpM-4NK=4RjOrQCETeUPeGG_7ftaNEinCA@mail.gmail.com>
 <CAO6_XqpLUSXcg9D=EZ-rJ97EMB0yqnpGuVKDk_8sy22kvFgNrQ@mail.gmail.com>
 <CAO6_Xqq18VZji7G8MMEd8EiuuY1FrsZo=V1ch71aU7uZL6yqjg@mail.gmail.com>
 <CAO6_XqppNRbiRDF608ZogmLLtwdA8ENak0QAy40fDM5U_zqrHA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="hphjdLxQZZh4aATV"
Content-Disposition: inline
In-Reply-To: <CAO6_XqppNRbiRDF608ZogmLLtwdA8ENak0QAy40fDM5U_zqrHA@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Z7Q1TTtOxYEiNn0h%40paquier.xyz>
Precedence: bulk


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

On Tue, Jan 14, 2025 at 09:49:02AM +0100, Anthonin Bonnefoy wrote:
> During my tests, I've noticed I didn't handle query cancellation, this
> is now fixed. I've also added additional comments related to
> available_results to make it clearer that it depends on what the
> server has flushed to the client.

This is a pretty cool patch.  I like the structure you have used for
the integration with the tracking of the number of commands, the
number of syncs (like pgbench) put in a pipeline, the number of
results requested and the number of results available.  That makes the
whole easier to look at with a state in pset.

+	PSQL_SEND_PIPELINE_SYNC,
+	PSQL_START_PIPELINE_MODE,
+	PSQL_END_PIPELINE_MODE,
+	PSQL_FLUSH,
+	PSQL_SEND_FLUSH_REQUEST,
+	PSQL_GET_RESULTS,

These new values are inconsistent, let's use some more PSQL_SEND_*
here.  That makes the whole set of values more greppable.

The tests in psql.sql are becoming really long.  Perhaps it would be
better to split that into its own file, say psql_pipeline.sql?  The
input file is already 2k lines, you are adding 15% more lines to that.

+ * otherwise, calling PQgetResults will block.=20

Likely PQgetResults =3D> PQgetResult().

Wondering if the cancellation in ExecQueryAndProcessResults() is
sound, I've not been able to break it, still..

I can also get behind the additions of \flush and \flushrequest to
query different parts of libpq.

+	if (pset.requested_results =3D=3D 0)
+		/* We've read all requested results, exit */
+		return res;

Set of nits with the style of the code, but I'd suggest to use
braces {} here, to outline that the comment is in a block.  There's a
second, larger one in discardAbortedPipelineResults().

+	if (strcmp(cmd, "gx") =3D=3D 0 && PQpipelineStatus(pset.db) !=3D PQ_PIPEL=
INE_OFF)
+	{
+		pg_log_error("\\gx not allowed in pipeline mode");
+		clean_extended_state();
+		return PSQL_CMD_ERROR;
+	}

What is the reasoning here behind this restriction?  \gx is a wrapper
of \g with expanded mode on, but it is also possible to call \g with
expanded=3Don, bypassing this restriction.

Let's split the prompt patch with the support of %P into its own
patch.

-#define DEFAULT_PROMPT1 "%/%R%x%# "
-#define DEFAULT_PROMPT2 "%/%R%x%# "
+#define DEFAULT_PROMPT1 "%/%R%P%x%# "
+#define DEFAULT_PROMPT2 "%/%R%P%x%# "
 #define DEFAULT_PROMPT3 ">> "

I don't think that changing this default is a good idea.  Everybody
can do that in their own .psqlrc (spoiler: I do).

The idea to use three fields with a hardcoded format does not look
like a good idea to me.  I think that this should be done in a
different and more extensible way:
- Use %P to track if we are in pipeline mode on, off or abort.
- Define three new variables that behave like ROW_COUNT, but for the
fields you want to track here.  These could then be passed down to a
PROMPT with %:name:, grammar already supported.

That would make the whole much more flexible.  At it seems to me that
we could also add requested_results to this set?  These could be named
with the same prefix, like PIPELINE_SYNC_COUNT, etc.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAme0NU0ACgkQnvQgOdby
QH38iA//UdPubIRO+L52gnX6T21Y7gn8lb3WZJBua93YsUV3XkzHdHEGNENDF1zm
u/2Cf8+7fKjgu/ePQO+dX0EEgEhYGVcoOdGCiFysrmTPOHWmH6oAiOspLiTCy7u5
jMI4Du7FVqI4A+mBEQOC904il5Pm/H8yaK1Tgy+PBti/ldSJdSVWHvej74TrI3DN
nq6Q+kkrrY0jwwvaSGmFUmTIxRLMO1hr/2ZqkghksRX1JX72zakEV07sW0uqBDW8
2I/vbdjNAuoNk3FOGi5s8FQKoXWtILQ52mnFt922UUyK1XgSnYDqdRRnE1HpbfZ6
JQqchOec8i2MqGEiBpGznPXYo8wneQceoX7lScWEIKGM397jPLHGVHsUExAwQUBa
tSqcBS8euf9KbDLiThfZGrDIYQWjOvJcTX3j6S2BTrl0obF3Hg3+GBKJ1lehCn3f
XDLI9JP0iYh32HKI5X+4JisrUkfwXIU3HeMlxtyxtE7aeV8mTEswCm87TcAKlDN5
czJFKfxiJBHF9tmrYawRLvwaXGFNjmt5X76M/J+64nKqWIV4v0u+OG7uGZa1SqSz
iOI/eehoX9ZMusZMglfzGcQU2m/1gESAJoPsDL8QNAh295Vqy4uNhu4e/D8Iva0l
Y6TmPiNiyAupa0UWQcc2Tbvh42sT1lmYV7mt4lSIEcHyO0U+Mis=
=qtQq
-----END PGP SIGNATURE-----

--hphjdLxQZZh4aATV--