Обсуждение: Add \syncpipeline command to pgbench

Hi,

PQsendSyncMessage() was added in
https://commitfest.postgresql.org/46/4262/. It allows users to add a
Sync message without flushing the buffer.

As a follow-up, this change adds an additional meta-command to
pgbench, \syncpipeline, which will call PQsendSyncMessage(). This will
make it possible to benchmark impact and improvements of using
PQsendSyncMessage through pgbench.

Regards,
Anthonin

On Thu, Jan 18, 2024 at 09:48:28AM +0100, Anthonin Bonnefoy wrote:
> PQsendSyncMessage() was added in
> https://commitfest.postgresql.org/46/4262/. It allows users to add a
> Sync message without flushing the buffer.
>
> As a follow-up, this change adds an additional meta-command to
> pgbench, \syncpipeline, which will call PQsendSyncMessage(). This will
> make it possible to benchmark impact and improvements of using
> PQsendSyncMessage through pgbench.

Thanks for sending that as a separate patch.

As a matter of fact, I have already looked at what you are proposing
here for the sake of the other thread when checking the difference in
numbers with PQsendSyncMessage().  The logic looks sound, but I have a
comment about the docs: could it be better to group \syncpipeline with
\startpipeline and \endpipeline?  \syncpipeline requires a pipeline to
work.
--
Michael

On Fri, Jan 19, 2024 at 5:08 AM Michael Paquier <michael@paquier.xyz> wrote:
> The logic looks sound, but I have a
> comment about the docs: could it be better to group \syncpipeline with
> \startpipeline and \endpipeline?  \syncpipeline requires a pipeline to
> work.

I've updated the doc to group the commands. It does look better and
more consistent with similar command groups like \if.

Regards,
Anthonin

On Fri, Jan 19, 2024 at 08:55:31AM +0100, Anthonin Bonnefoy wrote:
> I've updated the doc to group the commands. It does look better and
> more consistent with similar command groups like \if.

I was playing with a few meta command scenarios while looking at this
patch, and this sequence generates an error that should never happen:
$ cat /tmp/test.sql
\startpipeline
\syncpipeline
$ pgbench -n -f /tmp/test.sql -M extended
[...]
pgbench: error: unexpected transaction status 1
pgbench: error: client 0 aborted while receiving the transaction status

It looks to me that we need to be much smarter than that for the error
handling we'd need when a sync request is optionally sent when a
transaction stops at the end of pgbench.  Could you look at it?
--
Michael

That looks like a bug with how opened pipelines are not caught at the
end of the script processing. startpipeline seems to have similar
related issues.

$ cat only_startpipeline.sql
\startpipeline
SELECT 1;

With only 1 transaction, pgbench will consider this a success despite
not sending anything since the pipeline was not flushed:
pgbench -t1 -Mextended -f only_startpipeline.sql
[...]
number of transactions per client: 1
number of transactions actually processed: 1/1

With 2 transactions, the error will happen when \startpipeline is
called a second time:
pgbench -t2 -Mextended -f only_startpipeline.sql
[...]
pgbench: error: client 0 aborted in command 0 (startpipeline) of
script 0; already in pipeline mode
number of transactions per client: 2
number of transactions actually processed: 1/2

I've split the changes into two patches.
0001 introduces a new error when the end of a pgbench script is
reached while there's still an ongoing pipeline.
0002 adds the \syncpipeline command (original patch with an additional
test case).

Regards,
Anthonin

On Mon, Jan 22, 2024 at 7:16 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Jan 19, 2024 at 08:55:31AM +0100, Anthonin Bonnefoy wrote:
> > I've updated the doc to group the commands. It does look better and
> > more consistent with similar command groups like \if.
>
> I was playing with a few meta command scenarios while looking at this
> patch, and this sequence generates an error that should never happen:
> $ cat /tmp/test.sql
> \startpipeline
> \syncpipeline
> $ pgbench -n -f /tmp/test.sql -M extended
> [...]
> pgbench: error: unexpected transaction status 1
> pgbench: error: client 0 aborted while receiving the transaction status
>
> It looks to me that we need to be much smarter than that for the error
> handling we'd need when a sync request is optionally sent when a
> transaction stops at the end of pgbench.  Could you look at it?
> --
> Michael

On 2024-Jan-22, Anthonin Bonnefoy wrote:

> That looks like a bug with how opened pipelines are not caught at the
> end of the script processing. startpipeline seems to have similar
> related issues.

Ah, yeah.  Your fix looks necessary on a quick look.  I'll review and
see about backpatching this.

> 0002 adds the \syncpipeline command (original patch with an additional
> test case).

I can look into this one later, unless Michael wants to.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"No me acuerdo, pero no es cierto.  No es cierto, y si fuera cierto,
 no me acuerdo."                 (Augusto Pinochet a una corte de justicia)

On 2024-Jan-22, Anthonin Bonnefoy wrote:

> 0001 introduces a new error when the end of a pgbench script is
> reached while there's still an ongoing pipeline.

Pushed, backpatched to 14.  I reworded the error message to be

  client %d aborted: end of script reached with pipeline open

I hope this is OK.  I debated a half a dozen alternatives ("with open
pipeline", "without closing pipeline", "with unclosed pipeline" (???),
"leaving pipeline open") and decided this was the least ugly.

Thanks,

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
<inflex> really, I see PHP as like a strange amalgamation of C, Perl, Shell
<crab> inflex: you know that "amalgam" means "mixture with mercury",
       more or less, right?
<crab> i.e., "deadly poison"

On Mon, Jan 22, 2024 at 05:53:13PM +0100, Alvaro Herrera wrote:
> I hope this is OK.  I debated a half a dozen alternatives ("with open
> pipeline", "without closing pipeline", "with unclosed pipeline" (???),
> "leaving pipeline open") and decided this was the least ugly.

That looks OK to me.  Thanks for looking at that!
--
Michael

On Mon, Jan 22, 2024 at 01:59:00PM +0100, Alvaro Herrera wrote:
> On 2024-Jan-22, Anthonin Bonnefoy wrote:
>> 0002 adds the \syncpipeline command (original patch with an additional
>> test case).
>
> I can look into this one later, unless Michael wants to.

The patch seemed rather OK at quick glance as long as there is a test
to check for error path with a \syncpipeline still on the stack of
metacommands to handle.
Anyway, I wanted to study this one and learn a bit more about the
error stuff that was happening on pgbench side.  Now, if you feel
strongly about it, please go ahead!
--
Michael

On Tue, Jan 23, 2024 at 01:08:24PM +0900, Michael Paquier wrote:
> Anyway, I wanted to study this one and learn a bit more about the
> error stuff that was happening on pgbench side.

Well, I've spend some time studying this part, and the error handling
was looking correct based on the safety measures added in
49f7c6c44a5f, so I've just moved on and applied it.
--
Michael