Re: PATCH: Batch/pipelining support for libpq

On Mon, Oct 3, 2016 at 12:48 PM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> On 3 October 2016 at 10:10, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Tue, Sep 6, 2016 at 8:01 PM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>>> On 6 September 2016 at 16:10, Daniel Verite <daniel(at)manitou-mail(dot)org> wrote:
>>>> Craig Ringer wrote:
>>>>
>>>>> Updated patch attached.
>>>>
>>>> Please find attached a couple fixes for typos I've came across in
>>>> the doc part.
>>>
>>> Thanks, will apply and post a rebased patch soon, or if someone picks
>>> this up in the mean time they can apply your diff on top of the patch.
>>
>> Could you send an updated patch then? At the same time I am noticing
>> that git --check is complaining... This patch has tests and a
>> well-documented feature, so I'll take a look at it soon at the top of
>> my list. Moved to next CF for now.
>
> Thanks.
>
> I'd really like to teach psql in non-interactive mode to use it, but
> (a) I'm concerned about possible subtle behaviour differences arising
> if we do that and (b) I won't have the time. I think it's mostly of
> interest to app authors and driver developers and that's what it's
> aimed at. pg_restore could benefit a lot too.

Looking at it now... The most interesting comments are first.

I wanted to congratulate you. I barely see a patch with this level of
details done within the first versions. Anybody can review the patch
just by looking at the code and especially the docs without looking at
the thread. There are even tests to show what this does, for the
client.

+PQisInBatchMode 172
+PQqueriesInBatch 173
+PQbeginBatchMode 174
+PQendBatchMode 175
+PQsendEndBatch 176
+PQgetNextQuery 177
+PQbatchIsAborted 178
This set of routines is a bit inconsistent. Why not just prefixing
them with PQbatch? Like that for example:
PQbatchStatus(): consists of disabled/inactive/none, active, error.
This covers both PQbatchIsAborted() and PQisInBatchMode().
PQbatchBegin()
PQbatchEnd()
PQbatchQueueSync() or PQbatchQueueFlush, same as PQsendEndBatch() to
add and process a sync message into the queue.
PQbatchQueueCount(): returns N>0 if there are N entries, 0 if empty,
-1 on failure
PQbatchQueueProcess(): returns 1 if process can begin, 0 if not, -1 on
failure (OOM)

+ <para>
+ Much like asynchronous query mode, there is no performance disadvantage to
+ using batching and pipelining. It somewhat increased client application
+ complexity and extra caution is required to prevent client/server network
+ deadlocks, but can offer considerable performance improvements.
+ </para>
I would reword that a bit "it increases client application complexity
and extra caution is required to prevent client/server deadlocks but
offers considerable performance improvements".

+ ("ping time") is high, and when many small operations are being
performed in
Nit: should use <quote> here. Still not quoting it would be fine.

+ After entering batch mode the application dispatches requests
+ using normal asynchronous <application>libpq</application> functions like
+ <function>PQsendQueryParams</function>,
<function>PQsendPrepare</function>,
+ etc. The asynchronous requests are followed by a <link
It may be better to list all the functions here, PQSendQuery

* query work remains or an error has occurred (e.g. out of
* memory).
*/
-
PGresult *
PQgetResult(PGconn *conn)
Some noise in the patch.

git diff --check complains:
usage_exit(argv[0]);
warning: 1 line adds whitespace errors.

+++ b/src/interfaces/libpq/t/001_libpq_async.pl
@@ -0,0 +1,15 @@
+use strict;
+use warnings;
+
+use Config;
+use PostgresNode;
+use TestLib;
+use Test::More;
+
+my $node = get_new_node('main');
+$node->init;
+$node->start;
+
+my $port = $node->port;
+
+$node->stop('fast');
Er... This does nothing..

The changes in src/test/examples/ are not necessary anymore. You moved
all the tests to test_libpq (for the best actually).

+ while (queue != NULL)
+ {
+ PGcommandQueueEntry *prev = queue;
+ queue = queue->next;
+ free(prev);
+ }
This should free prev->query.

/* blah comment
* blah2 comment
*/
A lot of comment blocks are like that, those should be reformated.

Running directly make check in src/test/modules/test_libpq/ does not work:
# Postmaster PID for node "main" is 10225
# Running: testlibpqbatch dbname=postgres 10000 disallowed_in_batch
Command 'testlibpqbatch' not found in [...PATH list ...]
The problem is that testlibpqbatch is not getting compiled but I think
it should.

+ * testlibpqbatch.c
+ * Test of batch execution funtionality
[...]
+ fprintf(stderr, "%s is not a recognised test name\n", argv[3]);
s/funtionality/functionality/
s/recognised/recognized/

+ if (!PQsendQueryParams(conn, "SELECT $1", 1, dummy_param_oids,
+ dummy_params, NULL, NULL, 0))
+ {
+ fprintf(stderr, "dispatching first SELECT failed: %s\n",
PQerrorMessage(conn));
+ goto fail;
+ }
+
+ if (!PQsendEndBatch(conn))
+ {
+ fprintf(stderr, "Ending first batch failed: %s\n",
PQerrorMessage(conn));
+ goto fail;
+ }
+
+ if (!PQsendQueryParams(conn, "SELECT $1", 1, dummy_param_oids,
+ dummy_params, NULL, NULL, 0))
+ {
+ fprintf(stderr, "dispatching second SELECT failed: %s\n",
PQerrorMessage(conn));
+ goto fail;
+ }
May be better to use a loop here and set in the queue a bunch of queries..

You could just remove the VERBOSE flag in the tests, having a test
more talkative is always better.

+ <para>
+ The batch API was introduced in PostgreSQL 9.6, but clients using it can
+ use batches on server versions 8.4 and newer. Batching works on any server
+ that supports the v3 extended query protocol.
+ </para>
Postgres 10, not 9.6.

It may be a good idea to check for PG_PROTOCOL_MAJOR < 3 and issue an
error for the new routines.

+int
+PQgetNextQuery(PGconn *conn)
[...]
+ return false;
The new routines had better return explicitly 0 or 1, not a boolean
for consistency with the rest.

+ for processing. Returns 0 and has no effect if there are no query results
+ pending, batch mode is not enabled, or if the query currently processed
+ is incomplete or still has pending results. See <link
It would be useful for the user to make a difference between those
different statuses. As of your patch, PQgetNextQuery() returns false/0
in all those cases so it is a bit hard to know what's going on. Maybe
PQgetNextQuery() could be renamed to PQbatchGetNext, PQbatchQueueNext
to outline the fact that it is beginning the next query in the queue.
This helps also to understand that this can just be used with the
batch mode. Actually no, I am wrong. It is possible to guess each one
of those errors with respectively PQgetResult == NULL,
PQisInBatchMode() and PQqueriesInBatch(). Definitely it should be
mentioned in the docs that it is possible to make a difference between
all those states.

+ entry = PQmakePipelinedCommand(conn);
+ entry->queryclass = PGQUERY_SYNC;
+ entry->query = NULL;
PQmakePipelinedCommand() returns NULL, and boom.

+ bool in_batch; /* connection is in batch (pipelined) mode */
+ bool batch_aborted; /* current batch is aborted,
discarding until next Sync */
Having only one flag would be fine. batch_aborted is set of used only
when in_batch is used, so both have a strong link.

/* OK, it's launched! */
- conn->asyncStatus = PGASYNC_BUSY;
+ if (conn->in_batch)
+ PQappendPipelinedCommand(conn, pipeCmd);
+ else
+ conn->asyncStatus = PGASYNC_BUSY;
No, it is put in the queue.

+ <para>
+ Returns the number of queries still in the queue for this batch, not
+ including any query that's currently having results being processsed.
+ This is the number of times <function>PQgetNextQuery</function> has to be
+ called before the query queue is empty again.
+
+<synopsis>
+int PQqueriesInBatch(PGconn *conn);
+</synopsis>
This is not true. It does not return a count, just 0 or 1.

It may be a good idea to add a test for COPY and trigger a failure.

If I read the code correctly, it seems to me that it is possible to
enable the batch mode, and then to use PQexec(), PQsendPrepare will
just happily process queue the command. Shouldn't PQexec() be
prevented in batch mode? Similar remark for PQexecParams(),
PQexecPrepared() PQdescribePrepared and PQprepare(). In short
everything calling PQexecFinish().

I haven't run the tests directly on Windows wiht MSVC, but they won't
run as vcregress modulescheck lacks knowledge about modules using TAP.
I cannot blame you for that..
--
Michael