Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 20 Feb 2025 17:02:42 +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: <Z7bhohOpNVqh7TGc@paquier.xyz>
References: <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>
 <Z7Q1TTtOxYEiNn0h@paquier.xyz>
 <CAO6_XqrrjJRhKNhYa9Cg=_BSH8MmS1tAHMFq-H7fH=NcWJ-jvw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="FSmocRcgjJx8v3k5"
Content-Disposition: inline
In-Reply-To: <CAO6_XqrrjJRhKNhYa9Cg=_BSH8MmS1tAHMFq-H7fH=NcWJ-jvw@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Z7bhohOpNVqh7TGc%40paquier.xyz>
Precedence: bulk


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

On Tue, Feb 18, 2025 at 06:34:20PM +0100, Anthonin Bonnefoy wrote:
> On Tue, Feb 18, 2025 at 8:23=E2=80=AFAM Michael Paquier <michael@paquier.=
xyz> wrote:
>> 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.
>=20
> Agreed, I wasn't sure if this was enough to warrant a dedicated test
> file. This is now separated in psql_pipeline.sql.

You have forgotten the expected output.  Not a big issue as the input
was sent.

>> 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.
>=20
> The issue is that \gx enables expanded mode for the duration of the
> query and immediately reset it in sendquery_cleanup. With pipelining,
> the command is piped and displaying is done by either \endpipeline or
> \getresults, so the flag change has no impact. Forbidding it was a way
> to make it clearer that it won't have the expected effect. If we
> wanted a similar feature, this would need to be done with something
> like \endpipelinex or \getresultsx.

Hmm, okay.  If one wants one mode or the other it is always possible
to force one with \pset expanded when getting the results.  Not sure
if there is any need for new specific commands for these two printing
the results.  Another option would be to authorize the command to run,
but perhaps your option is just better as per the enforced behavior in
the output.  So fine by me.  There is coverage so we'll know if there
are arguments in favor of authorizing the command, if need be.

> I've split the patch and created the 3 special variables:
> PIPELINE_SYNC_COUNT, PIPELINE_COMMAND_COUNT, PIPELINE_RESULT_COUNT.

Thanks.  Looks sensible now.

> For requested_results, I don't think there's value in exposing it
> since it is used as an exit condition and thus will always be 0
> outside of ExecQueryAndProcessResults.

I've been playing with this patch and this configuration:
\set PROMPT1 '=3D(pipeline=3D%P,sync=3D%:PIPELINE_SYNC_COUNT:,cmd=3D%:PIPEL=
INE_COMMAND_COUNT:,res=3D%:PIPELINE_RESULT_COUNT:)%#'=20

That's long, but seeing the evolution of the pipeline status is pretty
cool depending on the meta-commands used.

While testing, I have been able to run into an assertion failure by
adding some tests in psql.sql to check for the case of inactive
branches for \if.  For example:
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -1047,11 +1047,15 @@ select \if false \\ (bogus \else \\ 42 \endif \\ fo=
rty_two;
    \echo arg1 arg2 arg3 arg4 arg5
    \echo arg1
    \encoding arg1
+   \endpipeline
    \errverbose

And the report:
+psql: mainloop.c:513: MainLoop: Assertion `conditional_active(cond_stack)'=
 failed.

We should have tests for all new six meta-commands in psql.sql.
MainLoop() is wrong when in pipeline mode for inactive branches.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAme24aIACgkQnvQgOdby
QH3iExAAhDz1A4VyiYNMWyK9auAtdher9q1pxpw8hh/NGgJihg3ZhMZB7MT2iUSp
V9DG9KajLdTIsebNC5AmQ0swROAxgYgtU18myMpX9MqEwpBoUftHzl9kJ4s3xUAf
Piwx7D1//G4SQVo5QWpSRGyWJ46pO6aKZ0KLAKpeaX93g5eIZ3JYi2jURSMzde7k
RgrB3oN0WZad+qONqMA2KMYW0UVaa7nzFrAdw83ua37tRJl9ApoBa4twdm4pQMsl
2CjQEhDARto0mbdZi9kV0Syytdx9jbIPTCwLHIscYsvB6JpUWWBqPjLDpAW3HL4y
IgDdUNNDJgseIsQh+P7eOhntM/kXxQbqrft95+rOeErnL3/AtxP7P58ULDuv/Sia
x9Bpx2iD/3N4XT6Md9ttxWJ0IdMG6nqzobijUW3ZMqQHILVfQL7Xh3rpFrPtCwLN
vkBRM1ZuqKtBF+KsLAf0Ao6tVxRE/aF4vN5vcVnZpIW1QH9I+pM/07Wg3msWv9+X
IpY6cfcrCk9kxU1YoQ1zEXR+YpDT1R2eouXGqTXZIEsFMksqfFveSIpM8C16jVLS
cwhhrWJChCKGCR7JKNN7pxqJ/Vpvly8oVUmGqPcsB88UeWG2UuYe/IiZ6QZxX2fF
YYez9GXu4IsL4OvHEmAtqNyGGgyxUpfqA0Sc97ZU3Bx3i11UfVk=
=044W
-----END PGP SIGNATURE-----

--FSmocRcgjJx8v3k5--