Re: PATCH: Batch/pipelining support for libpq

As you can see in an XXX comment in the libpq test program, the current
implementation has the behavior that PQgetResult() returns NULL after a
batch is finished and has reported PGRES_BATCH_END. I don't know if
there's a hard reason to do that, but I'd like to supress it because it
seems weird and out of place.

--
�lvaro Herrera 39�49'30"S 73�17'W

Hi,

+ commandFailed(st, "SQL", "\\gset and \\aset are not
allowed in a batch section");

It seems '\\gset or \\aset is not ' would correspond to the check more
closely.

+       if (my_command->argc != 1)
+           syntax_error(source, lineno, my_command->first_line,
my_command->argv[0],

It is possible that my_command->argc == 0 (where my_command->argv[0]
shouldn't be accessed) ?

+               appendPQExpBufferStr(&conn->errorMessage,
+                                 libpq_gettext("cannot queue commands
during COPY\n"));
+               return false;
+               break;

Is the break necessary ? Similar comment for pqBatchProcessQueue().

+int
+PQexitBatchMode(PGconn *conn)

Since either 0 or 1 is returned, maybe change the return type to bool.
Also, the function operates on PGconn - should the function be static
(pqBatchProcessQueue is) ?

Cheers

On Thu, Jan 21, 2021 at 3:39 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

On 2021-Jan-21, Alvaro Herrera wrote:

As you can see in an XXX comment in the libpq test program, the current
implementation has the behavior that PQgetResult() returns NULL after a
batch is finished and has reported PGRES_BATCH_END. I don't know if
there's a hard reason to do that, but I'd like to supress it because it
seems weird and out of place.

Hello Craig, a question for you since this API is your devising. I've
been looking at changing the way this works to prevent those NULL
returns from PQgetResult. That is, instead of having what seems like a
"NULL separator" of query results, you'd just get the PGRES_BATCH_END to
signify a batch end (not a NULL result after the BATCH_END); and the
normal PGRES_COMMAND_OK or PGRES_TUPLES_OK etc when the result of a
command has been sent. It's not working yet so I'm not sending an
updated patch, but I wanted to know if you had a rationale for including
this NULL return "separator" or was it just a convenience because of how
the code grew together.

Such a decision has obvious consequences for the test program (which
right now expects that PQgetResult() returns NULL at each step); and
naturally for libpq's internal state machine that controls how it all
works.

Thanks,

--
�lvaro Herrera 39�49'30"S 73�17'W

On Thu, 11 Feb 2021 at 07:51, Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

On 2021-Jan-21, Alvaro Herrera wrote:

As you can see in an XXX comment in the libpq test program, the current
implementation has the behavior that PQgetResult() returns NULL after a
batch is finished and has reported PGRES_BATCH_END. I don't know if
there's a hard reason to do that, but I'd like to supress it because it
seems weird and out of place.

Hello Craig, a question for you since this API is your devising. I've
been looking at changing the way this works to prevent those NULL
returns from PQgetResult. That is, instead of having what seems like a
"NULL separator" of query results, you'd just get the PGRES_BATCH_END to
signify a batch end (not a NULL result after the BATCH_END); and the
normal PGRES_COMMAND_OK or PGRES_TUPLES_OK etc when the result of a
command has been sent. It's not working yet so I'm not sending an
updated patch, but I wanted to know if you had a rationale for including
this NULL return "separator" or was it just a convenience because of how
the code grew together.

The existing API for libpq actually specifies[1]https://www.postgresql.org/docs/current/libpq-async.html#LIBPQ-PQSENDQUERY that you should call
PQgetResult() until it returns NULL:

After successfully calling PQsendQuery, call PQgetResult one or more

times to obtain the results. PQsendQuery cannot be called again (on the
same connection) until PQgetResult has returned a null pointer, indicating
that the command is done.

Similarly, in single-row mode, the existing API specifies that you should
call PQgetResult() until it returns NULL.

Also, IIRC the protocol already permits multiple result sets to be
returned, and the caller cannot safely assume that a single PQsendQuery()
will produce only a single result set. (I really should write a test
extension that exercises that and how libpq reacts to it.)

That's why I went for the NULL return. I think. It's been a while now so
it's a bit fuzzy.

I would definitely like an API that does not rely on testing for a NULL
return. Especially since NULL return can be ambiguous in the context of
row-at-a-time mode. New explicit enumerations for PGresult would make a lot
more sense.

So +1 from me for the general idea. I need to look at the patch as it has
evolved soon too.

Remember that the original patch I submitted for this was a 1-day weekend
hack and proof of concept to show that libpq could be modified to support
query pipelining (and thus batching too), so I could illustrate the
performance benefits that could be attained by doing so. I'd been aware of
the benefits and the protocol's ability to support it for some time thanks
to working on PgJDBC, but couldn't get anyone interested without some C
code to demonstrate it, so I wrote some. I am not going to argue that the
API I added for it is ideal in any way, and happy to see improvements.

The only change I would very strongly object to would be anything that
turned this into a *batch* mode without query-pipelining support. If you
have to queue all the queries up in advance then send them as a batch and
wait for all the results, you miss out on a lot of the potential
round-trip-time optimisations and you add initial latency. So long as there
is a way to "send A", "send B", "send C", "read results from A", "send D",
and there's a way for the application to associate some kind of state (an
application specific id or index, a pointer to an application query-queue
struct, whatever) so it can match queries to results ... then I'm happy.

[1]: https://www.postgresql.org/docs/current/libpq-async.html#LIBPQ-PQSENDQUERY
https://www.postgresql.org/docs/current/libpq-async.html#LIBPQ-PQSENDQUERY

On Tue, 16 Feb 2021 at 09:19, Craig Ringer <craig.ringer@enterprisedb.com>
wrote:

So long as there is a way to "send A", "send B", "send C", "read results
from A", "send D", and there's a way for the application to associate some
kind of state (an application specific id or index, a pointer to an
application query-queue struct, whatever) so it can match queries to
results ... then I'm happy.

FWIW I'm also thinking of revising the docs to mostly use the term
"pipeline" instead of "batch". Use "pipelining and batching" in the chapter
topic, and mention "batch" in the index, and add a para that explains how
to run batches on top of pipelining, but otherwise use the term "pipeline"
not "batch".

That's because pipelining isn't actually batching, and using it as a naïve
batch interface will get you in trouble. If you PQsendQuery(...) a long
list of queries without consuming results, you'll block unless you're in
libpq's nonblocking-send mode. You'll then be deadlocked because the server
can't send results until you consume some (tx buffer full) and can't
consume queries until it can send some results, but you can't consume
results because you're blocked on a send that'll never end.

An actual batch interface where you can bind and send a long list of
queries might be worthwhile, but should be addressed separately, and it'd
be confusing if this pipelining interface claimed the term "batch". I'm not
convinced enough application developers actually code directly against
libpq for it to be worth creating a pretty batch interface where you can
submit (say) an array of struct PQbatchEntry { const char * query, int
nparams, ... } to a PQsendBatch(...) and let libpq handle the socket I/O
for you. But if someone wants to add one later it'll be easier if we don't
use the term "batch" for the pipelining feature.

I'll submit a reworded patch in a bit.

On 2021-Feb-16, Craig Ringer wrote:

FWIW I'm also thinking of revising the docs to mostly use the term
"pipeline" instead of "batch". Use "pipelining and batching" in the chapter
topic, and mention "batch" in the index, and add a para that explains how
to run batches on top of pipelining, but otherwise use the term "pipeline"
not "batch".

Hmm, this is a good point. It means I have a lot of API renaming to do.
I'll get on it now and come back with a proposal.

--
�lvaro Herrera Valdivia, Chile

Here's a new version, where I've renamed everything to "pipeline". I
think the docs could use some additional tweaks now in order to make a
coherent story on pipeline mode, how it can be used in a batched
fashion, etc.

Here's the renames I applied. It's mostly mechanical, except
PQbatchSendQueue is now PQsendPipeline:

PQBatchStatus -> PGpipelineStatus (enum)
PQBATCH_MODE_OFF -> PQ_PIPELINE_OFF
PQBATCH_MODE_ON -> PQ_PIPELINE_ON
PQBATCH_MODE_ABORTED -> PQ_PIPELINE_ABORTED
PQbatchStatus -> PQpipelineStatus (function)
PQenterBatchMode -> PQenterPipelineMode
PQexitBatchMode -> PQexitPipelineMode
PQbatchSendQueue -> PQsendPipeline
PGRES_BATCH_END -> PGRES_PIPELINE_END
PGRES_BATCH_ABORTED -> PGRES_PIPELINE_ABORTED

Also, PQbatchStatus(conn) returns enum PGpipelineStatus (it previously
returned int).

I'm tempted to rename PGASYNC_QUEUED to PGASYNC_PIPELINE_IDLE (not sure
if PGASYNC_PIPELINE_READY fits better with the existing one).

In pgbench, I changed the metacommands to be \startpipeline and
\endpipeline. There's a failing Assert() there which I commented out;
needs fixed.

--
�lvaro Herrera 39�49'30"S 73�17'W

Hi,
+       if (querymode == QUERY_SIMPLE)
+       {
+           commandFailed(st, "startpipeline", "cannot use pipeline mode
with the simple query protocol");
+           st->state = CSTATE_ABORTED;
+           return CSTATE_ABORTED;

I wonder why the st->state is only assigned for this if block. The state is
not set for other cases where CSTATE_ABORTED is returned.

Should PQ_PIPELINE_OFF be returned for the following case ?

+PQpipelineStatus(const PGconn *conn)
+{
+   if (!conn)
+       return false;

Cheers

On Tue, Feb 16, 2021 at 3:14 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

Hello, thanks for looking at this patch.

On 2021-Feb-16, Zhihong Yu wrote:

+       if (querymode == QUERY_SIMPLE)
+       {
+           commandFailed(st, "startpipeline", "cannot use pipeline mode
with the simple query protocol");
+           st->state = CSTATE_ABORTED;
+           return CSTATE_ABORTED;

I wonder why the st->state is only assigned for this if block. The state is
not set for other cases where CSTATE_ABORTED is returned.

Yeah, that's a simple oversight. We don't need to set st->state,
because the caller sets it to the value we return.

Should PQ_PIPELINE_OFF be returned for the following case ?

+PQpipelineStatus(const PGconn *conn)
+{
+   if (!conn)
+       return false;

Yep.

Thanks

--
�lvaro Herrera 39�49'30"S 73�17'W

On 2021-Jan-21, Zhihong Yu wrote:

It seems '\\gset or \\aset is not ' would correspond to the check more
closely.

+       if (my_command->argc != 1)
+           syntax_error(source, lineno, my_command->first_line,
my_command->argv[0],

It is possible that my_command->argc == 0 (where my_command->argv[0]
shouldn't be accessed) ?

No -- process_backslash_command reads the word and sets it as argv[0].

+               appendPQExpBufferStr(&conn->errorMessage,
+                                 libpq_gettext("cannot queue commands
during COPY\n"));
+               return false;
+               break;

Is the break necessary ? Similar comment for pqBatchProcessQueue().

Not necessary. I removed them.

+int
+PQexitBatchMode(PGconn *conn)

Since either 0 or 1 is returned, maybe change the return type to bool.

I was kinda tempted to do that, but in reality a lot of libpq's API is
defined like that -- to return 0 (failure)/1 (success) as ints, not
bools. For example see PQsendQuery(). I'm not inclined to do different
for these new functions. (One curious case is PQsetvalue, which returns
int, and is documented as "returns zero for failure" and then uses
"return false").

Also, the function operates on PGconn - should the function be static
(pqBatchProcessQueue is) ?

I don't understand this suggestion. How would an application call it,
if we make it static?

Thanks

--
�lvaro Herrera Valdivia, Chile
"C�mo ponemos nuestros dedos en la arcilla del otro. Eso es la amistad; jugar
al alfarero y ver qu� formas se pueden sacar del otro" (C. Halloway en
La Feria de las Tinieblas, R. Bradbury)

Hi,
+static int pqBatchProcessQueue(PGconn *conn);

I was suggesting changing:

+int
+PQexitBatchMode(PGconn *conn)

to:

+static int
+PQexitBatchMode(PGconn *conn)

Cheers

On Fri, Feb 19, 2021 at 10:43 AM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

Hi,

On 2021-Feb-19, Zhihong Yu wrote:

Hi,
+static int pqBatchProcessQueue(PGconn *conn);

I was suggesting changing:

+int
+PQexitBatchMode(PGconn *conn)

to:

+static int
+PQexitBatchMode(PGconn *conn)

I understand that, but what I'm saying is that it doesn't work.
pqBatchProcessQueue can be static because it's only used internally in
libpq, but PQexitBatchMode is supposed to be called by the client
application.

--
�lvaro Herrera Valdivia, Chile
"No necesitamos banderas
No reconocemos fronteras" (Jorge Gonz�lez)

Hi,
Thanks for the response.

On Fri, Feb 19, 2021 at 12:46 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

On Wed, 17 Feb 2021, 07:13 Alvaro Herrera, <alvherre@alvh.no-ip.org> wrote:

Here's a new version, where I've renamed everything to "pipeline".

Wow. Thanks.

I

think the docs could use some additional tweaks now in order to make a
coherent story on pipeline mode, how it can be used in a batched
fashion, etc.

I'll do that soon and send an update.

On 2021-Feb-20, Craig Ringer wrote:

On Wed, 17 Feb 2021, 07:13 Alvaro Herrera, <alvherre@alvh.no-ip.org> wrote:

I
think the docs could use some additional tweaks now in order to make a
coherent story on pipeline mode, how it can be used in a batched
fashion, etc.

I'll do that soon and send an update.

I started to do that, but was sidetracked having a look at error
handling while checking one of the claims.

So now it starts with this:

<sect3 id="libpq-pipeline-sending">
<title>Issuing Queries</title>

<para>
After entering pipeline mode, the application dispatches requests using
<xref linkend="libpq-PQsendQueryParams"/>,
or its prepared-query sibling
<xref linkend="libpq-PQsendQueryPrepared"/>.
These requests are queued on the client-side until flushed to the server;
this occurs when <xref linkend="libpq-PQsendPipeline"/> is used to
establish a synchronization point in the pipeline,
or when <xref linkend="libpq-PQflush"/> is called.
The functions <xref linkend="libpq-PQsendPrepare"/>,
<xref linkend="libpq-PQsendDescribePrepared"/>, and
<xref linkend="libpq-PQsendDescribePortal"/> also work in pipeline mode.
Result processing is described below.
</para>

<para>
The server executes statements, and returns results, in the order the
client sends them. The server will begin executing the commands in the
pipeline immediately, not waiting for the end of the pipeline.
If any statement encounters an error, the server aborts the current
transaction and skips processing commands in the pipeline until the
next synchronization point established by <function>PQsendPipeline</function>.
(This remains true even if the commands in the pipeline would rollback
the transaction.)
Query processing resumes after the synchronization point.
</para>

(Note I changed the wording that "the pipeline is ended by
PQsendPipeline" to "a synchronization point is established". Is this
easily understandable? On re-reading it, I'm not sure it's really an
improvement.)

BTW we don't explain why doesn't PQsendQuery work (to wit: because it
uses "simple" query protocol and thus doesn't require the Sync message).
I think we should either document that, or change things so that
PQsendQuery no longer uses a 'Q' message when in pipeline mode; instead
use extended query protocol. I don't see why we'd force people to use
PQsendQueryParams when not necessary.

BTW, in my experimentation, the sequence of PGresult that you get when
handling a pipeline with errors don't make a lot of sense. I'll spend
some more time on it.

While at it, as for the PGresult sequence and NULL returns: I think for
PQexec and maybe PQsendQuery, it makes sense to loop until PQgetResult
returns NULL, because you never know how many results are you going to
get. But for PQsendQueryParams, this is no longer true, because you
can't send multiple queries in one query string. So the only way to get
multiple results, is by using single-row mode. But that already has its
own protocol for multiple results, namely to get a stream of
PGRES_SINGLE_TUPLE terminated by a zero-rows PGRES_TUPLES_OK. So I'm
not sure there is a strong need for the mandatory NULL result.

--
�lvaro Herrera 39�49'30"S 73�17'W

On Wed, Feb 17, 2021 8:14 AM (JST), Alvaro Herrera wrote:

Hi Alvaro,

Here's a new version, where I've renamed everything to "pipeline". I think the
docs could use some additional tweaks now in order to make a coherent
story on pipeline mode, how it can be used in a batched fashion, etc.

I tried applying this patch to test it on top of Iwata-san's libpq trace log [1]/messages/by-id/1d650278-552a-4bd2-8411-a907fd54446c@www.fastmail.com.
In my environment, the compiler complains.
It seems that in libpqwalreceiver.c: libpqrcv_exec()
the switch for PQresultStatus needs to handle the
cases for PGRES_PIPELINE_END and PGRES_PIPELINE_ABORTED too.

switch (PQresultStatus(pgres))
{
...
Case PGRES_PIPELINE_ABORTED:
...
Case PGRES_PIPELINE_ END:

Regards,
Kirk Jamison

[1]: /messages/by-id/1d650278-552a-4bd2-8411-a907fd54446c@www.fastmail.com

On 2021-Mar-03, k.jamison@fujitsu.com wrote:

I tried applying this patch to test it on top of Iwata-san's libpq trace log [1].
In my environment, the compiler complains.
It seems that in libpqwalreceiver.c: libpqrcv_exec()
the switch for PQresultStatus needs to handle the
cases for PGRES_PIPELINE_END and PGRES_PIPELINE_ABORTED too.

Yeah, I noticed this too and decided to handle these cases as errors.

Here's v28, which includes that fix, and also gets rid of the
PGASYNC_QUEUED state; I instead made the rest of the code work using the
existing PGASYNC_IDLE state. This allows us get rid of a few cases
where we had to report "internal error: IDLE state unexpected" in a few
cases, and was rather pointless. With the current formulation, both
pipeline mode and normal mode share pretty much all state.

Also, I've renamed PGRES_PIPELINE_END to PGRES_PIPELINE_SYNC, because
that's closer to what it means: that result state by no means that
pipeline mode has ended. It only means that you called PQsendPipeline()
so the server gives you a Sync message.

I'm much more comfortable with this version, so I'm marking the patch as
Ready for Committer in case anybody else wants to look at this before I
push it.

However: on errors, I think it's a bit odd that you get a NULL from
PQgetResult after getting PGRES_PIPELINE_ABORTED. Maybe I should
suppress that. I'm no longer wrestling with the NULL after
PGRES_PIPELINE_END however, because that's not critical and you can not
ask for it and things work fine.

(Also, the test pipeline.c program in src/test/modules/test_libpq has a
new "transaction" mode that is not exercised by the TAP program; I'll
plug that in before commit.)

--
�lvaro Herrera Valdivia, Chile

On 2021-Mar-03, 'Alvaro Herrera' wrote:

I'm much more comfortable with this version, so I'm marking the patch as
Ready for Committer in case anybody else wants to look at this before I
push it.

Actually, I just noticed a pretty serious problem, which is that when we
report an error, we don't set "conn->errQuery" to the current query but
to the *previous* query, leading to non-sensical error reports like

$ ./pipeline pipeline_abort
aborted pipeline... ERROR: no existe la funci�n no_such_function(integer)
LINE 1: INSERT INTO pq_pipeline_demo(itemno) VALUES ($1);
^
HINT: Ninguna funci�n coincide en el nombre y tipos de argumentos. Puede ser necesario agregar conversi�n expl�cita de tipos.
ok

(Sorry about the Spanish there, but the gist of the problem is that
we're positioning the error cursor on a query that succeeded prior to
the query that raised the error.)

This should obviously not occur. I'm trying to figure out how to repair
it and not break everything again ...

--
�lvaro Herrera Valdivia, Chile

On 2021-Mar-03, 'Alvaro Herrera' wrote:

This should obviously not occur. I'm trying to figure out how to repair
it and not break everything again ...

I think trying to set up the connection state so that the next query
appears in conn->last_query prior to PQgetResult being called again
leads to worse breakage. The simplest fix seems to make fe-protocol3.c
aware that in this case, the query is in conn->cmd_queue_head instead,
as in the attached patch.

--
�lvaro Herrera 39�49'30"S 73�17'W