Add PQsendSyncMessage() to libpq

Anton Kirilov wrote:

I would appeciate your thoughts on my proposal.

This sounds like a useful addition to me. I've played a bit with it in
Psycopg and it works fine.

diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index a16bbf32ef..e2b32c1379 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -82,6 +82,7 @@ static int    PQsendDescribe(PGconn *conn, char desc_type,
  static int     check_field_number(const PGresult *res, int field_num);
  static void pqPipelineProcessQueue(PGconn *conn);
  static int     pqPipelineFlush(PGconn *conn);
+static int     send_sync_message(PGconn *conn, int flush);

Could (should?) be:
static int send_sync_message(PGconn *conn, bool flush);

diff --git a/src/test/modules/libpq_pipeline/libpq_pipeline.c 
b/src/test/modules/libpq_pipeline/libpq_pipeline.c
index f48da7d963..829907957a 100644
--- a/src/test/modules/libpq_pipeline/libpq_pipeline.c
+++ b/src/test/modules/libpq_pipeline/libpq_pipeline.c
@@ -244,6 +244,104 @@ test_multi_pipelines(PGconn *conn)
         fprintf(stderr, "ok\n");
  }

+static void
+test_multi_pipelines_noflush(PGconn *conn)
+{

Maybe test_multi_pipelines() could be extended with an additional
PQsendQueryParams()+PQsendSyncMessage() step instead of adding this
extra test case?

Hello,

On 25/04/2023 15:23, Denis Laxalde wrote:

This sounds like a useful addition to me. I've played a bit with it in
Psycopg and it works fine.

Thank you very much for reviewing my patch! I have attached a new
version of it that addresses your comments and that has been rebased on
top of the current tip of the master branch (by fixing a merge
conflict), i.e. commit 7b7fa85130330128b404eddebd4f33c6739454b0.

For the sake of others who might read this e-mail thread, I would like
to mention that my patch is complete (including documentation and tests,
but modulo review comments, of course), and that it passes the tests, i.e.:

make check
make -C src/test/modules/libpq_pipeline check

Best wishes,
Anton Kirilov

Hello,

Anton Kirilov a écrit :

On 25/04/2023 15:23, Denis Laxalde wrote:

This sounds like a useful addition to me. I've played a bit with it in
Psycopg and it works fine.

Thank you very much for reviewing my patch! I have attached a new
version of it that addresses your comments and that has been rebased on
top of the current tip of the master branch (by fixing a merge
conflict), i.e. commit 7b7fa85130330128b404eddebd4f33c6739454b0.

For the sake of others who might read this e-mail thread, I would like
to mention that my patch is complete (including documentation and tests,
but modulo review comments, of course), and that it passes the tests, i.e.:

make check
make -C src/test/modules/libpq_pipeline check

Thank you; this V2 looks good to me.
Marking as ready for committer.

On Thu, Apr 27, 2023 at 01:06:27PM +0200, Denis Laxalde wrote:

Thank you; this V2 looks good to me.
Marking as ready for committer.

Please note that we are in a stabilization period for v16 and that the
first commit fest of v17 should start in July, so it will perhaps take
some time before this is looked at by a committer.

Speaking of which, what was the performance impact of your application
once PQflush() was moved out of the pipeline sync? Just asking for
curiosity..
--
Michael

Michael Paquier a écrit :

On Thu, Apr 27, 2023 at 01:06:27PM +0200, Denis Laxalde wrote:

Thank you; this V2 looks good to me.
Marking as ready for committer.

Please note that we are in a stabilization period for v16 and that the
first commit fest of v17 should start in July, so it will perhaps take
some time before this is looked at by a committer.

Yes, I am aware; totally fine by me.

Speaking of which, what was the performance impact of your application
once PQflush() was moved out of the pipeline sync? Just asking for
curiosity..

I have no metrics for that; but maybe Anton has some?
(In Psycopg, we generally do not expect users to handle the sync
operation themselves, it's done under the hood; and I only found one
situation where the flush could be avoided, but that's largely because
our design, there can be more in general I think.)

On Fri, Mar 24, 2023 at 6:39 PM Anton Kirilov <antonvkirilov@gmail.com> wrote:

I have attached a patch that introduces PQsendSyncMessage(), a
function that is equivalent to PQpipelineSync(), except that it does
not flush anything to the server; the user must subsequently call
PQflush() instead. Alternatively, the new function is equivalent to
PQsendFlushRequest(), except that it sends a sync message instead of a
flush request. In addition to reducing the system call overhead of
libpq's pipeline mode, it also makes it easier for the operating
system to send as much of the pipeline as possible in a single TCP (or
lower level protocol) packet when the database is running remotely.

I wonder whether this is the naming that we want. The two names are
significantly different. Something like PQpipelineSendSync() would be
more similar.

I also wonder, really even more, whether it would be better to do
something like PQpipelinePutSync(PGconn *conn, bool flush) with
PQpipelineSync(conn) just meaning PQpipelinePutSync(conn, true). We're
basically using the function name as a Boolean parameter to select the
behavior, which is fine if you only have one parameter and it's a
Boolean, but it's obviously unworkable if you have say 3 Boolean
parameters because you don't want 8 different functions, and what if
you need an integer parameter for some reason?

So I'd favor exposing a function that is effectively an extended
version of PQpipelineSendSync() with an additional Boolean parameter,
and that way if for some reason somebody needs to extend it again,
they can just make an even more extended version with yet another
parameter. That way, all the functionality is always available by
calling the newest function, and older ones are still there for older
applications.

--
Robert Haas
EDB: http://www.enterprisedb.com

Hello,

On 28/04/2023 13:06, Robert Haas wrote:

On Fri, Mar 24, 2023 at 6:39 PM Anton Kirilov <antonvkirilov@gmail.com> wrote:

I have attached a patch that introduces PQsendSyncMessage()...

I wonder whether this is the naming that we want. The two names are
significantly different. Something like PQpipelineSendSync() would be
more similar.

The reason is that the function is modeled after PQsendFlushRequest(),
since it felt closer to what I was trying to achieve, i.e. appending a
protocol message to the output buffer without doing any actual I/O
operations.

I also wonder, really even more, whether it would be better to do
something like PQpipelinePutSync(PGconn *conn, bool flush) with
PQpipelineSync(conn) just meaning PQpipelinePutSync(conn, true).

Actually I believe that there is another issue with PQpipelineSync()
that has to do with ergonomics - according to a comment inside its body
(
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/interfaces/libpq/fe-exec.c;h=a16bbf32ef5c0043eee9c92ab82bf4f11386ee47;hb=HEAD#l3189
) it could fail silently to send all the buffered data, which seems to
be problematic when operating in non-blocking mode. In practice, this
means that all calls to PQpipelineSync() must be followed by execution
of PQflush() to check whether the application should poll for write
readiness. I suppose that that was the reason why I was going for a
solution that did not combine changing the connection state with doing
I/O operations.

In any case I am not particularly attached to any naming or the exact
shape of the new API, as long as it achieves the same goal (reducing the
number of system calls), but before I make any substantial changes to my
patch, I would like to hear your thoughts on the matter.

Best wishes,
Anton Kirilov

Hello,

On 28/04/2023 09:08, Denis Laxalde wrote:

Michael Paquier a écrit :

Speaking of which, what was the performance impact of your application
once PQflush() was moved out of the pipeline sync?  Just asking for
curiosity..

I have no metrics for that; but maybe Anton has some?

I did a quick check using the TechEmpower Framework Benchmarks (
https://www.techempower.com/benchmarks/ ) - they define 4 Web
application tests that are database-bound. Everything was running on a
single machine, and 3 of the tests had an improvement of 29.16%, 32.30%,
and 41.78% respectively in the number of requests per second (Web
application requests, not database queries), while the last test
regressed by 0.66% (which I would say is practically no difference,
given that there is always some measurement noise). I will try to get
the changes from my patch tested in the project's continuous
benchmarking environment, which has a proper set up with 3 servers
(client, application server, and database) connected by a 10GbE link.

Best wishes,
Anton Kirilov

On Sun, Apr 30, 2023 at 01:59:17AM +0100, Anton Kirilov wrote:

I did a quick check using the TechEmpower Framework Benchmarks (
https://www.techempower.com/benchmarks/ ) - they define 4 Web application
tests that are database-bound. Everything was running on a single machine,
and 3 of the tests had an improvement of 29.16%, 32.30%, and 41.78%
respectively in the number of requests per second (Web application requests,
not database queries), while the last test regressed by 0.66% (which I would
say is practically no difference, given that there is always some
measurement noise). I will try to get the changes from my patch tested in
the project's continuous benchmarking environment, which has a proper set up
with 3 servers (client, application server, and database) connected by a
10GbE link.

Well, these are nice numbers. At ~1% I am ready to buy the noise
argument, but what would the range of the usual noise when it comes to
multiple runs under the same conditions?

Let's make sure that the API interface is the most intuitive (Robert
has commented about that a few days ago, still need to follow up on
that).
--
Michael

On Sat, Apr 29, 2023 at 05:06:03PM +0100, Anton Kirilov wrote:

In any case I am not particularly attached to any naming or the exact shape
of the new API, as long as it achieves the same goal (reducing the number of
system calls), but before I make any substantial changes to my patch, I
would like to hear your thoughts on the matter.

Another thing that may matter in terms of extensibility? Would a
boolean argument really be the best design? Could it be better to
have instead one API with a bits32 and some flags controlling its
internals?
--
Michael

On Mon, May 1, 2023 at 8:42 PM Michael Paquier <michael@paquier.xyz> wrote:

Another thing that may matter in terms of extensibility? Would a
boolean argument really be the best design? Could it be better to
have instead one API with a bits32 and some flags controlling its
internals?

I wondered that, too. If we never add any more Boolean parameters to
this function then that would end up a waste, but maybe we will and
then it will be genius. Not sure what's best.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 2023-May-02, Robert Haas wrote:

On Mon, May 1, 2023 at 8:42 PM Michael Paquier <michael@paquier.xyz> wrote:

Another thing that may matter in terms of extensibility? Would a
boolean argument really be the best design? Could it be better to
have instead one API with a bits32 and some flags controlling its
internals?

I wondered that, too. If we never add any more Boolean parameters to
this function then that would end up a waste, but maybe we will and
then it will be genius. Not sure what's best.

I agree that adding a flag is the way to go, since it improve chances
that we won't end up with ten different functions in case we decide to
have eight other behaviors. One more function and we're done. And
while I can't think of any use for a future flag, we (I) already didn't
of this one either, so let's not make the same mistake.

We already have 'int' flag masks in PQcopyResult() and
PQsetTraceFlags(). We were using bits32 initially for flag stuff in the
PQtrace facilities, until [1]/messages/by-id/TYAPR01MB2990B6C6A32ACF15D97AE94AFEBD0@TYAPR01MB2990.jpnprd01.prod.outlook.com reminded us that we shouldn't let c.h
creep into app-land, so that was turned into plain 'int'.

[1]: /messages/by-id/TYAPR01MB2990B6C6A32ACF15D97AE94AFEBD0@TYAPR01MB2990.jpnprd01.prod.outlook.com

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"No nos atrevemos a muchas cosas porque son difíciles,
pero son difíciles porque no nos atrevemos a hacerlas" (Séneca)

Hello,

On 3 May 2023, at 11:03, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2023-May-02, Robert Haas wrote:

On Mon, May 1, 2023 at 8:42 PM Michael Paquier <michael@paquier.xyz> wrote:

Another thing that may matter in terms of extensibility? Would a
boolean argument really be the best design? Could it be better to
have instead one API with a bits32 and some flags controlling its
internals?

I wondered that, too. If we never add any more Boolean parameters to
this function then that would end up a waste, but maybe we will and
then it will be genius. Not sure what's best.

I agree that adding a flag is the way to go, since it improve chances
that we won't end up with ten different functions in case we decide to
have eight other behaviors. One more function and we're done. And
while I can't think of any use for a future flag, we (I) already didn't
of this one either, so let's not make the same mistake.

Thank you all for the feedback! Do you have any thoughts on the other issue with PQpipelineSync() I have mentioned in my previous message? Am I just misunderstanding what the code comment means and how the API is supposed to be used by any chance?

Best wishes,
Anton Kirilov

On 2023-May-04, Anton Kirilov wrote:

Thank you all for the feedback! Do you have any thoughts on the other
issue with PQpipelineSync() I have mentioned in my previous message?

Eh, I hadn't seen that one.

Am I just misunderstanding what the code comment means and how the API
is supposed to be used by any chance?

I think you have it right: it is possible that the buffer has not been
fully flushed by the time PQpipelineSync returns.

If you want to make sure it's fully flushed, your only option is to have
the call block. That would make it no longer non-blocking, so it has to
be explicitly requested behavior.
I think this means to add yet another behavior flag for the new
function: have it block, waiting for the buffer to be flushed.

So your application can put several sync points in the queue, with no
flushing (and of course no blocking), and have it flush+block only on
the "last" one. Of course, for other users, you want the current
behavior: have it flush opportunistically but not block. So you can't
make it a single flag.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

Hello,

On Thu, 4 May 2023, 11:36 Alvaro Herrera, <alvherre@alvh.no-ip.org <mailto:alvherre@alvh.no-ip.org>> wrote:

On 2023-May-04, Anton Kirilov wrote:
If you want to make sure it's fully flushed, your only option is to have
the call block.

Surely PQflush() returning 0 would signify that the output buffer has been fully flushed? Which means that there is another, IMHO simpler option than introducing an extra flag - make the new function return the same values as PQflush(), i.e. 0 for no error and fully flushed output, -1 for error, and 1 for partial flush (so that the user may start polling for write readiness). Of course, the function would never return 1 (but would block instead) unless the user has called PQsetnonblocking() beforehand.

Best wishes,
Anton Kirilov

On Wed, May 03, 2023 at 12:03:57PM +0200, Alvaro Herrera wrote:

We already have 'int' flag masks in PQcopyResult() and
PQsetTraceFlags(). We were using bits32 initially for flag stuff in the
PQtrace facilities, until [1] reminded us that we shouldn't let c.h
creep into app-land, so that was turned into plain 'int'.

[1] /messages/by-id/TYAPR01MB2990B6C6A32ACF15D97AE94AFEBD0@TYAPR01MB2990.jpnprd01.prod.outlook.com

Indeed. Good point!
--
Michael

Hello,

On 05/05/2023 16:02, Anton Kirilov wrote:

On Thu, 4 May 2023, 11:36 Alvaro Herrera, <alvherre@alvh.no-ip.org
<mailto:alvherre@alvh.no-ip.org>> wrote:

On 2023-May-04, Anton Kirilov wrote:
If you want to make sure it's fully flushed, your only option is to have
the call block.

Surely PQflush() returning 0 would signify that the output buffer has
been fully flushed?

Since I haven't got any further comments, I assume that I am correct, so
here is an updated version of the patch that should address all feedback
that I have received so far and all issues that I have identified.

Thanks,
Anton Kirilov

Hello,

On 02/05/2023 00:55, Michael Paquier wrote:

Well, these are nice numbers. At ~1% I am ready to buy the noise
argument, but what would the range of the usual noise when it comes to
multiple runs under the same conditions>

I managed to get my patch tested in the TechEmpower Framework Benchmarks
continuous benchmarking environment, and even though it takes roughly a
week to get a new set of results, now there had been a couple of runs
both with and without my changes. All 4 database-bound Web application
tests (single query, multiple queries, fortunes, and data updates) saw
improvements, by approximately 8.94%, 0.64%, 9.54%, and 2.78%
respectively. The standard errors were 0.65% or less, so there was
practically no change in the second test. However, I have seen another
implementation experience a much larger improvement (~6.69%) in that
test from essentially the same optimization, so I think that my own code
has another bottleneck. Note that these test runs were not in the same
benchmarking environment as the one I used previously for a quick check,
so the values differ. Also, another set of results should become
available in a week or so (and would be based on my optimization).

Links to the test runs:
https://www.techempower.com/benchmarks/#section=test&amp;runid=1ecf679a-9686-4de7-a3b7-de16a1a84bb6&amp;l=zik0zi-35r&amp;w=zhb2tb-zik0zj-zik0zj-sf&amp;test=db
https://www.techempower.com/benchmarks/#section=test&amp;runid=aab00736-445c-4b7f-83b5-451c47c83395&amp;l=zik0zi-35r&amp;w=zhb2tb-zik0zj-zik0zj-sf&amp;test=db
https://www.techempower.com/benchmarks/#section=test&amp;runid=bc7f7570-a88e-48e3-9874-06d7dc0a0f74&amp;l=zik0zi-35r&amp;w=zhb2tb-zik0zj-zik0zj-sf&amp;test=db
https://www.techempower.com/benchmarks/#section=test&amp;runid=e6dd1abd-7aa2-4846-9b44-d8fd8a23d385&amp;l=zik0zi-35r&amp;w=zhb2tb-zik0zj-zik0zj-sf&amp;test=db
(ordered chronologically; the first 2 did not include my optimization)

Best wishes,
Anton Kirilov

On 21 May 2023, at 19:17, Anton Kirilov <antonvkirilov@gmail.com> wrote:

.. here is an updated version of the patch

This hunk here:

-	if (PQflush(conn) < 0)
+	const int ret = flags & PG_PIPELINEPUTSYNC_FLUSH ? PQflush(conn) : 0;
+
+	if (ret < 0)

..is causing this compiler warning:

fe-exec.c: In function ‘PQpipelinePutSync’:
fe-exec.c:3203:2: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
3203 | const int ret = flags & PG_PIPELINEPUTSYNC_FLUSH ? PQflush(conn) : 0;
| ^~~~~
cc1: all warnings being treated as errors

Also, the patch no longer applies. Please rebase and send an updated version.

--
Daniel Gustafsson