Subject: Re: Add PQsendSyncMessage() to libpq

From: Anton Kirilov <[email-masked]>
Date: Sun, 12 Nov 2023 13:37:16 +0000

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

From b752269b2763f8d66bcfc79faf751e52226c344b Mon Sep 17 00:00:00 2001
From: Anton Kirilov <[email-masked]>
Date: Wed, 22 Mar 2023 20:39:57 +0000
Subject: [PATCH v5] Add PQsendPipelineSync() to libpq

This new function is equivalent to PQpipelineSync(), except
that it does not flush anything to the server; the user must
subsequently call PQflush() instead. Its purpose is to reduce
the system call overhead of pipeline mode.
---
 doc/src/sgml/libpq.sgml                       | 45 ++++++++++++++++---
 src/interfaces/libpq/exports.txt              |  1 +
 src/interfaces/libpq/fe-exec.c                | 17 +++++--
 src/interfaces/libpq/libpq-fe.h               |  1 +
 .../modules/libpq_pipeline/libpq_pipeline.c   | 37 +++++++++++++++
 .../traces/multi_pipelines.trace              | 11 +++++
 6 files changed, 102 insertions(+), 10 deletions(-)

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 64b2910fee..61bee82a54 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -3547,8 +3547,9 @@ ExecStatusType PQresultStatus(const PGresult *res);
           <listitem>
            <para>
             The <structname>PGresult</structname> represents a
-            synchronization point in pipeline mode, requested by
-            <xref linkend="libpq-PQpipelineSync"/>.
+            synchronization point in pipeline mode, requested by either
+            <xref linkend="libpq-PQpipelineSync"/> or
+            <xref linkend="libpq-PQsendPipelineSync"/>.
             This status occurs only when pipeline mode has been selected.
            </para>
           </listitem>
@@ -5122,7 +5123,8 @@ int PQsendClosePortal(PGconn *conn, const char *portalName);
        <xref linkend="libpq-PQsendDescribePrepared"/>,
        <xref linkend="libpq-PQsendDescribePortal"/>,
        <xref linkend="libpq-PQsendClosePrepared"/>,
-       <xref linkend="libpq-PQsendClosePortal"/>, or
+       <xref linkend="libpq-PQsendClosePortal"/>,
+       <xref linkend="libpq-PQsendPipelineSync"/>, or
        <xref linkend="libpq-PQpipelineSync"/>
        call, and returns it.
        A null pointer is returned when the command is complete and there
@@ -5506,8 +5508,9 @@ int PQflush(PGconn *conn);
      client sends them.  The server will begin executing the commands in the
      pipeline immediately, not waiting for the end of the pipeline.
      Note that results are buffered on the server side; the server flushes
-     that buffer when a synchronization point is established with
-     <function>PQpipelineSync</function>, or when
+     that buffer when a synchronization point is established with either
+     <function>PQpipelineSync</function> or
+     <function>PQsendPipelineSync</function>, or when
      <function>PQsendFlushRequest</function> is called.
      If any statement encounters an error, the server aborts the current
      transaction and does not execute any subsequent command in the queue
@@ -5564,7 +5567,8 @@ int PQflush(PGconn *conn);
      <type>PGresult</type> types <literal>PGRES_PIPELINE_SYNC</literal>
      and <literal>PGRES_PIPELINE_ABORTED</literal>.
      <literal>PGRES_PIPELINE_SYNC</literal> is reported exactly once for each
-     <function>PQpipelineSync</function> at the corresponding point
+     <function>PQpipelineSync</function> or
+     <function>PQsendPipelineSync</function> at the corresponding point
      in the pipeline.
      <literal>PGRES_PIPELINE_ABORTED</literal> is emitted in place of a normal
      query result for the first error and all subsequent results
@@ -5602,7 +5606,8 @@ int PQflush(PGconn *conn);
      <function>PQresultStatus</function> will report a
      <literal>PGRES_PIPELINE_ABORTED</literal> result for each remaining queued
      operation in an aborted pipeline. The result for
-     <function>PQpipelineSync</function> is reported as
+     <function>PQpipelineSync</function> or
+     <function>PQsendPipelineSync</function> is reported as
      <literal>PGRES_PIPELINE_SYNC</literal> to signal the end of the aborted pipeline
      and resumption of normal result processing.
     </para>
@@ -5834,6 +5839,32 @@ int PQsendFlushRequest(PGconn *conn);
        </para>
       </listitem>
      </varlistentry>
+
+    <varlistentry id="libpq-PQsendPipelineSync">
+     <term><function>PQsendPipelineSync</function><indexterm><primary>PQsendPipelineSync</primary></indexterm></term>
+
+     <listitem>
+      <para>
+       Marks a synchronization point in a pipeline by sending a
+       <link linkend="protocol-flow-ext-query">sync message</link>
+       without flushing the send buffer. This serves as
+       the delimiter of an implicit transaction and an error recovery
+       point; see <xref linkend="libpq-pipeline-errors"/>.
+
+<synopsis>
+int PQsendPipelineSync(PGconn *conn);
+</synopsis>
+      </para>
+      <para>
+       Returns 1 for success. Returns 0 if the connection is not in
+       pipeline mode or sending a
+       <link linkend="protocol-flow-ext-query">sync message</link>
+       failed.
+       Note that the message is not itself flushed to the server automatically;
+       use <function>PQflush</function> if necessary.
+      </para>
+     </listitem>
+    </varlistentry>
    </variablelist>
   </sect2>
 
diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
index 850734ac96..f5aa641001 100644
--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -191,3 +191,4 @@ PQclosePrepared           188
 PQclosePortal             189
 PQsendClosePrepared       190
 PQsendClosePortal         191
+PQsendPipelineSync        192
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index 04610ccf5e..c0dcb04282 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -3224,6 +3224,16 @@ pqPipelineProcessQueue(PGconn *conn)
  */
 int
 PQpipelineSync(PGconn *conn)
+{
+	return PQsendPipelineSync(conn) && pqFlush(conn) >= 0;
+}
+
+/*
+ * PQsendPipelineSync
+ *		Send a Sync message as part of a pipeline without flushing to server
+ */
+int
+PQsendPipelineSync(PGconn *conn)
 {
 	PGcmdQueueEntry *entry;
 
@@ -3267,10 +3277,11 @@ PQpipelineSync(PGconn *conn)
 		goto sendFailed;
 
 	/*
-	 * Give the data a push.  In nonblock mode, don't complain if we're unable
-	 * to send it all; PQgetResult() will do any additional flushing needed.
+	 * Give the data a push if we're past the size threshold. In nonblock
+	 * mode, don't complain if we're unable to send it all; the caller is
+	 * expected to execute PQflush() at some point anyway.
 	 */
-	if (PQflush(conn) < 0)
+	if (pqPipelineFlush(conn) < 0)
 		goto sendFailed;
 
 	/* OK, it's launched! */
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 97762d56f5..b0d89a592d 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -474,6 +474,7 @@ extern int	PQenterPipelineMode(PGconn *conn);
 extern int	PQexitPipelineMode(PGconn *conn);
 extern int	PQpipelineSync(PGconn *conn);
 extern int	PQsendFlushRequest(PGconn *conn);
+extern int	PQsendPipelineSync(PGconn *conn);
 
 /* LISTEN/NOTIFY support */
 extern PGnotify *PQnotifies(PGconn *conn);
diff --git a/src/test/modules/libpq_pipeline/libpq_pipeline.c b/src/test/modules/libpq_pipeline/libpq_pipeline.c
index 3c009ee153..23fb80986b 100644
--- a/src/test/modules/libpq_pipeline/libpq_pipeline.c
+++ b/src/test/modules/libpq_pipeline/libpq_pipeline.c
@@ -169,6 +169,14 @@ test_multi_pipelines(PGconn *conn)
 	if (PQpipelineSync(conn) != 1)
 		pg_fatal("Pipeline sync failed: %s", PQerrorMessage(conn));
 
+	if (PQsendQueryParams(conn, "SELECT $1", 1, dummy_param_oids,
+						  dummy_params, NULL, NULL, 0) != 1)
+		pg_fatal("dispatching first SELECT failed: %s", PQerrorMessage(conn));
+
+	/* Skip flushing once. */
+	if (PQsendPipelineSync(conn) != 1)
+		pg_fatal("Pipeline sync failed: %s", PQerrorMessage(conn));
+
 	if (PQsendQueryParams(conn, "SELECT $1", 1, dummy_param_oids,
 						  dummy_params, NULL, NULL, 0) != 1)
 		pg_fatal("dispatching second SELECT failed: %s", PQerrorMessage(conn));
@@ -206,6 +214,35 @@ test_multi_pipelines(PGconn *conn)
 
 	/* second pipeline */
 
+	res = PQgetResult(conn);
+	if (res == NULL)
+		pg_fatal("PQgetResult returned null when there's a pipeline item: %s",
+				 PQerrorMessage(conn));
+
+	if (PQresultStatus(res) != PGRES_TUPLES_OK)
+		pg_fatal("Unexpected result code %s from first pipeline item",
+				 PQresStatus(PQresultStatus(res)));
+	PQclear(res);
+	res = NULL;
+
+	if (PQgetResult(conn) != NULL)
+		pg_fatal("PQgetResult returned something extra after first result");
+
+	if (PQexitPipelineMode(conn) != 0)
+		pg_fatal("exiting pipeline mode after query but before sync succeeded incorrectly");
+
+	res = PQgetResult(conn);
+	if (res == NULL)
+		pg_fatal("PQgetResult returned null when sync result expected: %s",
+				 PQerrorMessage(conn));
+
+	if (PQresultStatus(res) != PGRES_PIPELINE_SYNC)
+		pg_fatal("Unexpected result code %s instead of sync result, error: %s",
+				 PQresStatus(PQresultStatus(res)), PQerrorMessage(conn));
+	PQclear(res);
+
+	/* third pipeline */
+
 	res = PQgetResult(conn);
 	if (res == NULL)
 		pg_fatal("PQgetResult returned null when there's a pipeline item: %s",
diff --git a/src/test/modules/libpq_pipeline/traces/multi_pipelines.trace b/src/test/modules/libpq_pipeline/traces/multi_pipelines.trace
index 4b9ab07ca4..1ee21f61dc 100644
--- a/src/test/modules/libpq_pipeline/traces/multi_pipelines.trace
+++ b/src/test/modules/libpq_pipeline/traces/multi_pipelines.trace
@@ -8,6 +8,17 @@ F	19	Bind	 "" "" 0 1 1 '1' 1 0
 F	6	Describe	 P ""
 F	9	Execute	 "" 0
 F	4	Sync
+F	21	Parse	 "" "SELECT $1" 1 NNNN
+F	19	Bind	 "" "" 0 1 1 '1' 1 0
+F	6	Describe	 P ""
+F	9	Execute	 "" 0
+F	4	Sync
+B	4	ParseComplete
+B	4	BindComplete
+B	33	RowDescription	 1 "?column?" NNNN 0 NNNN 4 -1 0
+B	11	DataRow	 1 1 '1'
+B	13	CommandComplete	 "SELECT 1"
+B	5	ReadyForQuery	 I
 B	4	ParseComplete
 B	4	BindComplete
 B	33	RowDescription	 1 "?column?" NNNN 0 NNNN 4 -1 0
-- 
2.34.1

---

From: Anthonin Bonnefoy <[email-masked]>
Date: Mon, 13 Nov 2023 09:19:52 +0100

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/047b5b05169e36361fe29fef9f430da045ef012d).
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=:aid1;
select 2;
\syncpipeline
select 1;
select * from pgbench_accounts where aid=:aid2;
select 2;
\endpipeline" > /tmp/pipeline_without_flush.sql
pgbench -T30 -Mextended -f /tmp/pipeline_without_flush.sql -h127.0.0.1
latency average = 0.383 ms
initial connection time = 2.810 ms
tps = 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=:aid1;
select 2;
\endpipeline
\startpipeline
select 1;
select * from pgbench_accounts where aid=:aid2;
select 2;
\endpipeline" > /tmp/pipeline_with_flush.sql
pgbench -T30 -Mextended -f /tmp/pipeline_with_flush.sql -h127.0.0.1
latency average = 0.437 ms
initial connection time = 2.602 ms
tps = 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

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Fri, 29 Dec 2023 12:49:29 +0100

> here is the updated version of the patch.

The new patch looks great to me. And indeed consensus seems to have
been reached on the approach and that this patch is useful. So I'm
taking the liberty of marking this patch as Ready for Committer.

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Fri, 29 Dec 2023 12:52:30 +0100

On Mon, 13 Nov 2023 at 09:20, Anthonin Bonnefoy
<[email-masked]> wrote:
> ...

Those are some nice improvements. And I think once this patch is in,
it would make sense to add the pgbench feature you're suggesting.
Because indeed that makes it see what perf improvements can be gained
for your workload.

---

From: Michael Paquier <[email-masked]>
Date: Sun, 31 Dec 2023 09:37:31 +0900

> Because indeed that makes it see what perf improvements can be gained

Yeah, that sounds like a good idea seen from here.  (Still need to
look at the core patch.)
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Wed, 10 Jan 2024 15:40:36 +0900

[...]

I was looking at this patch, and calling PQpipelineSync() would now
cause two calls of PQflush() to be issued when the output buffer
threshold has been reached.  Could that lead to regressions?

A second thing I find disturbing is that pqAppendCmdQueueEntry() would
be called before the final pqFlush(), which could cause the commands
to be listed in a queue even if the flush fails when calling
PQpipelineSync().

Hence, as a whole, wouldn't it be more consistent if the new
PQsendPipelineSync() and the existing PQpipelineSync() call an
internal static routine (PQPipelineSyncInternal?) that can switch
between both modes?  Let's just make the extra argument a boolean.
--
Michael

---

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

> between both modes?  Let's just make the extra argument a boolean.

Yeah, I'll go with that after a second look.  Attached is what I am
finishing with, and I have reproduced some numbers with the pgbench
metacommand mentioned upthread, which is reeeaaally nice.

I have also made a few edits to the tests.
--
Michael
From b653759f8dfdfe83d0d8bc1d4a0ac9d4e272a061 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Mon, 15 Jan 2024 16:15:00 +0900
Subject: [PATCH v6] Add PQsendPipelineSync() to libpq

This new function is equivalent to PQpipelineSync(), except
that it does not flush anything to the server; the user must
subsequently call PQflush() instead. Its purpose is to reduce
the system call overhead of pipeline mode.
---
 src/interfaces/libpq/exports.txt              |  1 +
 src/interfaces/libpq/fe-exec.c                | 41 +++++++++++++++--
 src/interfaces/libpq/libpq-fe.h               |  1 +
 .../modules/libpq_pipeline/libpq_pipeline.c   | 43 ++++++++++++++++++
 .../traces/multi_pipelines.trace              | 11 +++++
 doc/src/sgml/libpq.sgml                       | 45 ++++++++++++++++---
 6 files changed, 131 insertions(+), 11 deletions(-)

diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
index 28b861fd93..088592deb1 100644
--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -192,3 +192,4 @@ PQclosePortal             189
 PQsendClosePrepared       190
 PQsendClosePortal         191
 PQchangePassword          192
+PQsendPipelineSync        193
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index 106d14e6ee..9e7e670921 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -81,6 +81,7 @@ static int	PQsendTypedCommand(PGconn *conn, char command, char type,
 							   const char *target);
 static int	check_field_number(const PGresult *res, int field_num);
 static void pqPipelineProcessQueue(PGconn *conn);
+static int	pqPipelineSyncInternal(PGconn *conn, bool immediate_flush);
 static int	pqPipelineFlush(PGconn *conn);

@@ -3224,6 +3225,25 @@ pqPipelineProcessQueue(PGconn *conn)
 /*
  * PQpipelineSync
  *		Send a Sync message as part of a pipeline, and flush to server
+ */
+int
+PQpipelineSync(PGconn *conn)
+{
+	return pqPipelineSyncInternal(conn, true);
+}
+
+/*
+ * PQsendPipelineSync
+ *		Send a Sync message as part of a pipeline, without flushing to server
+ */
+int
+PQsendPipelineSync(PGconn *conn)
+{
+	return pqPipelineSyncInternal(conn, false);
+}
+
+/*
+ * Wrapper for PQpipelineSync and PQsendPipelineSync.
  *
  * It's legal to start submitting more commands in the pipeline immediately,
  * without waiting for the results of the current pipeline. There's no need to
@@ -3240,9 +3260,12 @@ pqPipelineProcessQueue(PGconn *conn)
  * The connection will remain in pipeline mode and unavailable for new
  * synchronous command execution functions until all results from the pipeline
  * are processed by the client.
+ *
+ * immediate_flush controls if the flush happens immediately after sending the
+ * Sync message or not.
  */
-int
-PQpipelineSync(PGconn *conn)
+static int
+pqPipelineSyncInternal(PGconn *conn, bool immediate_flush)
 {
 	PGcmdQueueEntry *entry;
 
@@ -3288,9 +3311,19 @@ PQpipelineSync(PGconn *conn)
 	/*
 	 * Give the data a push.  In nonblock mode, don't complain if we're unable
 	 * to send it all; PQgetResult() will do any additional flushing needed.
+	 * If immediate_flush is disabled, the data is pushed if we are past the
+	 * size threshold.
 	 */
-	if (PQflush(conn) < 0)
-		goto sendFailed;
+	if (immediate_flush)
+	{
+		if (pqFlush(conn) < 0)
+			goto sendFailed;
+	}
+	else
+	{
+		if (pqPipelineFlush(conn) < 0)
+			goto sendFailed;
+	}
 
 	/* OK, it's launched! */
 	pqAppendCmdQueueEntry(conn, entry);
index f0ec660cb6..defc415fa3 100644
 
 /* LISTEN/NOTIFY support */
 extern PGnotify *PQnotifies(PGconn *conn);
diff --git a/src/test/modules/libpq_pipeline/libpq_pipeline.c b/src/test/modules/libpq_pipeline/libpq_pipeline.c
index 71cd04c5f2..c68e20d0b5 100644
--- a/src/test/modules/libpq_pipeline/libpq_pipeline.c
+++ b/src/test/modules/libpq_pipeline/libpq_pipeline.c
@@ -162,6 +162,7 @@ test_multi_pipelines(PGconn *conn)
 	if (PQenterPipelineMode(conn) != 1)
 		pg_fatal("failed to enter pipeline mode: %s", PQerrorMessage(conn));
 
+	/* first pipeline */
 	if (PQsendQueryParams(conn, "SELECT $1", 1, dummy_param_oids,
 						  dummy_params, NULL, NULL, 0) != 1)
 		pg_fatal("dispatching first SELECT failed: %s", PQerrorMessage(conn));
@@ -169,6 +170,16 @@ test_multi_pipelines(PGconn *conn)
 	if (PQpipelineSync(conn) != 1)
 		pg_fatal("Pipeline sync failed: %s", PQerrorMessage(conn));
 
+	/* second pipeline */
@@ -177,6 +188,9 @@ test_multi_pipelines(PGconn *conn)
 		pg_fatal("pipeline sync failed: %s", PQerrorMessage(conn));
 
 	/* OK, start processing the results */
+
+	/* first pipeline */
+
 	res = PQgetResult(conn);
 	if (res == NULL)
 		pg_fatal("PQgetResult returned null when there's a pipeline item: %s",
@@ -206,6 +220,35 @@ test_multi_pipelines(PGconn *conn)
 
 	/* second pipeline */
 
index 21195e0e72..6d1d477276 100644
@@ -5507,8 +5509,9 @@ int PQflush(PGconn *conn);
@@ -5565,7 +5568,8 @@ int PQflush(PGconn *conn);
@@ -5603,7 +5607,8 @@ int PQflush(PGconn *conn);
@@ -5835,6 +5840,32 @@ int PQsendFlushRequest(PGconn *conn);
 
-- 
2.43.0

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Mon, 15 Jan 2024 10:01:59 +0100

> metacommand mentioned upthread, which is reeeaaally nice.

Code looks good to me. But one small notes on the test.

+     /* second pipeline */
+     if (PQsendQueryParams(conn, "SELECT $1", 1, dummy_param_oids,
+                                     dummy_params, NULL, NULL, 0) != 1)
+           pg_fatal("dispatching first SELECT failed: %s",
PQerrorMessage(conn));

Error message should be "second SELECT" not "first SELECT". Same note
for the error message in the third pipeline, where it still says
"second SELECT".

+           pg_fatal("PQgetResult returned null when there's a
pipeline item: %s",

same issue: s/first/second/g (and s/second/third/g for the existing
part of the test).

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 15 Jan 2024 10:49:56 +0100

On 2024-Jan-15, Michael Paquier wrote:

Looks good!  Just some small notes,

>   * without waiting for the results of the current pipeline. There's no need to

the new function pqPipelineSyncInternal is not a wrapper for these other
two functions -- the opposite is true actually.  We tend to use the term
"workhorse" or "internal workhorse" for this kind of thing.

In the docs, after this patch we have

- PQpipelineSync
- PQsendFlushRequest
- PQsendPipelineSync

Wouldn't it make more sense to add the new function in the middle of the
two existing ones instead?

Looking again at the largish comment that's now atop
pqPipelineSyncInternal(), I think most of it should be removed -- these
things should be explained in the SGML docs, and I think they are, in
the "Using Pipeline Mode" section.  We can just have the lines this
patch is adding.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"I can't go to a restaurant and order food because I keep looking at the
fonts on the menu.  Five minutes later I realize that it's also talking
about food" (Donald Knuth)

---

From: Michael Paquier <[email-masked]>
Date: Tue, 16 Jan 2024 08:28:25 +0900

> same issue: s/first/second/g (and s/second/third/g for the existing

Ugh, yes.  The note in the test was wrong.  Thanks for
double-checking.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Tue, 16 Jan 2024 12:32:22 +0900

> "workhorse" or "internal workhorse" for this kind of thing.

Indeed, makes sense.

> two existing ones instead?

Ordering PQsendPipelineSync just after PQpipelineSync is OK by me.
I've applied the patch with all these modifications to move on with
the subject.

> the "Using Pipeline Mode" section.  We can just have the lines this

Hmm.  The first two sentences about being able to submit more commands
to the pipeline are documented in the subsection "Issuing Queries".
The third sentence is implied in the second paragraph of this
subsection.  The 4th paragraph of the comment where sync commands
cannot be issued until all the results from the pipeline have been
consumed is mentioned in the first paragraph in "Using Pipeline Mode".
So you are right that this could be entirely removed.

How about the attached to remove all that, then?
--
Michael
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index 52d41658c1..152b100624 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -3245,23 +3245,6 @@ PQsendPipelineSync(PGconn *conn)
 /*
  * Workhorse function for PQpipelineSync and PQsendPipelineSync.
  *
- * It's legal to start submitting more commands in the pipeline immediately,
- * without waiting for the results of the current pipeline. There's no need to
- * end pipeline mode and start it again.
- *
- * If a command in a pipeline fails, every subsequent command up to and
- * including the result to the Sync message sent by pqPipelineSyncInternal
- * gets set to PGRES_PIPELINE_ABORTED state. If the whole pipeline is
- * processed without error, a PGresult with PGRES_PIPELINE_SYNC is produced.
- *
- * Queries can already have been sent before pqPipelineSyncInternal is called,
- * but pqPipelineSyncInternal needs to be called before retrieving command
- * results.
- *
- * The connection will remain in pipeline mode and unavailable for new
- * synchronous command execution functions until all results from the pipeline
- * are processed by the client.
- *
  * immediate_flush controls if the flush happens immediately after sending the
  * Sync message or not.
  */

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 16 Jan 2024 14:55:12 +0100

> I've applied the patch with all these modifications to move on with

Thanks!

> So you are right that this could be entirely removed.

(I'm pretty sure that the history of this comment is that Craig Ringer
wrote it for his prototype patch, and then I took the various parts and
struggled to add them as SGML docs as it made logical sense.  So if
there's anything in the comment that's important and not covered by the
docs, that would be a docs bug.)  I agree with your findings.

> How about the attached to remove all that, then?

Looks good, thank you.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Tiene valor aquel que admite que es un cobarde" (Fernandel)

---

From: Michael Paquier <[email-masked]>
Date: Wed, 17 Jan 2024 16:30:34 +0900

>> How about the attached to remove all that, then?

Thanks for double-checking.  Done.
--
Michael

---

From: Anton Kirilov <[email-masked]>
Date: Thu, 18 Jan 2024 23:11:22 +0000

Hello,

> Thanks for double-checking.  Done.
Thank you very much for taking care of my patch!

One thing that I noticed is that the TODO list on the PostgreSQL Wiki 
still contained an entry ( https://wiki.postgresql.org/wiki/Todo#libpq ) 
about adding pipelining support to libpq - perhaps it ought to be updated?

Best wishes,
Anton Kirilov