MIME-Version: 1.0
References: <202311081620.s2a7ao5jtlpe@alvherre.pgsql> <b878b8d2-8481-4fde-a374-5ccb9168d4e4@gmail.com>
In-Reply-To: <b878b8d2-8481-4fde-a374-5ccb9168d4e4@gmail.com>
From: Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com>
Date: Mon, 13 Nov 2023 09:19:52 +0100
Message-ID: <CAO6_Xqo_H3-ykt_Hmip-TKNBVWZbRthLvFu4qTpQSSEgV4dWmA@mail.gmail.com>
Subject: Re: Add PQsendSyncMessage() to libpq
To: Anton Kirilov <antonvkirilov@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>, Jelte Fennema-Nio <postgres@jeltef.nl>, 
	Michael Paquier <michael@paquier.xyz>, Robert Haas <robertmhaas@gmail.com>, 
	pgsql-hackers@postgresql.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAO6_Xqo_H3-ykt_Hmip-TKNBVWZbRthLvFu4qTpQSSEgV4dWmA%40mail.gmail.com>
Precedence: bulk

Hi,

I've played a bit with the patch on my side. One thing that would be
great would be to make this
available in pgbench through a \syncpipeline meta command. That would
make it easier for users
to test whether there's a positive impact with their queries or not.

I've wrote a patch to add it to pgbench (don't want to mess with the
thread's attachment so here's a GH link
https://github.com/bonnefoa/postgres/commit/047b5b05169e36361fe29fef9f430da=
045ef012d).
Here's some quick results:

echo "\set aid1 random(1, 100000 * :scale)
\set aid2 random(1, 100000 * :scale)
\startpipeline
select 1;
select * from pgbench_accounts where aid=3D:aid1;
select 2;
\syncpipeline
select 1;
select * from pgbench_accounts where aid=3D:aid2;
select 2;
\endpipeline" > /tmp/pipeline_without_flush.sql
pgbench -T30 -Mextended -f /tmp/pipeline_without_flush.sql -h127.0.0.1
latency average =3D 0.383 ms
initial connection time =3D 2.810 ms
tps =3D 2607.587877 (without initial connection time)

echo "\set aid1 random(1, 100000 * :scale)
\set aid2 random(1, 100000 * :scale)
\startpipeline
select 1;
select * from pgbench_accounts where aid=3D:aid1;
select 2;
\endpipeline
\startpipeline
select 1;
select * from pgbench_accounts where aid=3D:aid2;
select 2;
\endpipeline" > /tmp/pipeline_with_flush.sql
pgbench -T30 -Mextended -f /tmp/pipeline_with_flush.sql -h127.0.0.1
latency average =3D 0.437 ms
initial connection time =3D 2.602 ms
tps =3D 2290.527462 (without initial connection time)

I took some perfs and the main change is from the server spending less time=
 in
ReadCommand which makes sense since the commands are sent in a single tcp
frame with the \syncpipeline version.

Regards,
Anthonin

On Sun, Nov 12, 2023 at 2:37=E2=80=AFPM Anton Kirilov <antonvkirilov@gmail.=
com> wrote:
>
> Hello,
>
> Thanks for the feedback!
>
> On 07/11/2023 09:23, Jelte Fennema-Nio wrote:
>  > But I think it's looking at the situation from the wrong direction.
> [...] we should look at it as an addition to our current list of PQsend
> functions for a new packet type. And none of those PQsend functions ever
> needed a flag. Which makes sense, because they are the lowest level
> building blocks that make sense from a user perspective: They send a
> message type over the socket and don't do anything else.
>
> Yes, I think that this is quite close to my thinking when I created the
> original version of the patch. Also, the protocol specification states
> that the Sync message lacks parameters.
>
> Since there haven't been any comments from the other people who have
> chimed in on this e-mail thread, I will assume that there is consensus
> (we are doing a U-turn with the implementation approach after all), so
> here is the updated version of the patch.
>
> Best wishes,
> Anton Kirilov