Subject: Add \syncpipeline command to pgbench

From: Anthonin Bonnefoy <[email-masked]>
Date: Thu, 18 Jan 2024 09:48:28 +0100

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

---

From: Michael Paquier <[email-masked]>
Date: Fri, 19 Jan 2024 13:08:11 +0900

> 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

---

From: Anthonin Bonnefoy <[email-masked]>
Date: Fri, 19 Jan 2024 08:55:31 +0100

On Fri, Jan 19, 2024 at 5:08 AM Michael Paquier <[email-masked]> wrote:
> The logic looks sound, but I have a

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

Regards,
Anthonin

---

From: Michael Paquier <[email-masked]>
Date: Mon, 22 Jan 2024 15:16:25 +0900

> 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

---

From: Anthonin Bonnefoy <[email-masked]>
Date: Mon, 22 Jan 2024 10:11:20 +0100

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

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 22 Jan 2024 13:59:00 +0100

> end of the script processing. startpipeline seems to have similar

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

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)

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 22 Jan 2024 17:53:13 +0100

> 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"

---

From: Michael Paquier <[email-masked]>
Date: Tue, 23 Jan 2024 12:57:09 +0900

> "leaving pipeline open") and decided this was the least ugly.

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

---

From: Michael Paquier <[email-masked]>
Date: Tue, 23 Jan 2024 13:08:24 +0900

> 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

---

From: Michael Paquier <[email-masked]>
Date: Wed, 24 Jan 2024 17:13:53 +0900

On Tue, Jan 23, 2024 at 01:08:24PM +0900, Michael Paquier wrote:
> 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