Re: PATCH: Batch/pipelining support for libpq

> +
> +  <para>
> +   <application>libpq</application> supports queueing up multiple queries into
> +   a pipeline to be executed as a batch on the server. Batching queries allows
> +   applications to avoid a client/server round-trip after each query to get
> +   the results before issuing the next query.
> +  </para>

"queueing .. into a pipeline" sounds weird to me - but I'm not a native
speaker.  Also batching != pipelining.

> +  <sect2>
> +   <title>When to use batching</title>
> +
> +   <para>
> +    Much like asynchronous query mode, there is no performance disadvantage to
> +    using batching and pipelining. It increases client application complexity
> +    and extra caution is required to prevent client/server deadlocks but
> +    offers considerable performance improvements.
> +   </para>

s/offers/can sometimes offer/

> +  <sect2 id="libpq-batch-using">
> +   <title>Using batch mode</title>
> +
> +   <para>
> +    To issue batches the application must switch
> +    <application>libpq</application> into batch mode.

s/libpq/a connection/?

> Enter batch mode with <link
> +    linkend="libpq-PQbatchBegin"><function>PQbatchBegin(conn)</function></link> or test
> +    whether batch mode is active with <link
> +    linkend="libpq-PQbatchStatus"><function>PQbatchStatus(conn)</function></link>. In batch mode only <link
> +    linkend="libpq-async">asynchronous operations</link> are permitted, and
> +    <literal>COPY</literal> is not recommended as it most likely will trigger failure in batch processing.
> +    (The restriction on <literal>COPY</literal> is an implementation
> +    limit; the PostgreSQL protocol and server can support batched
> <literal>COPY</literal>).

Hm, I'm unconvinced that that's a useful parenthetical in the libpq
docs.

> +   <para>
> +    The client uses libpq's asynchronous query functions to dispatch work,
> +    marking the end of each batch with <function>PQbatchQueueSync</function>.
> +    Concurrently, it uses <function>PQgetResult</function> and
> +    <function>PQbatchQueueProcess</function> to get results.

"Concurrently" imo is a dangerous word, somewhat implying multi-threading.

> +   <note>
> +    <para>
> +     It is best to use batch mode with <application>libpq</application> in
> +     <link linkend="libpq-pqsetnonblocking">non-blocking mode</link>. If used in
> +     blocking mode it is possible for a client/server deadlock to occur. The
> +     client will block trying to send queries to the server, but the server will
> +     block trying to send results from queries it has already processed to the
> +     client. This only occurs when the client sends enough queries to fill its
> +     output buffer and the server's receive buffer before switching to
> +     processing input from the server, but it's hard to predict exactly when
> +     that'll happen so it's best to always use non-blocking mode.
> +    </para>
> +   </note>

Such deadlocks are possible just as well with non-blocking mode, unless
one can avoid sending queries and switching to receiving results anytime
in the application code.

> +    <para>
> +     Batched operations will be executed by the server in the order the client
> +     sends them. The server will send the results in the order the statements
> +     executed. The server may begin executing the batch before all commands
> +     in the batch are queued and the end of batch command is sent. If any
> +     statement encounters an error the server aborts the current transaction and
> +     skips processing the rest of the batch. Query processing resumes after the
> +     end of the failed batch.
> +    </para>

What if a batch contains transaction boundaries?

> +   <sect3 id="libpq-batch-results">
> +    <title>Processing results</title>
> +
> +    <para>
> +     The client <link linkend="libpq-batch-interleave">interleaves result
> +     processing with sending batch queries</link>, or for small batches may
> +     process all results after sending the whole batch.
> +    </para>

That's a very long <link> text, is it not?

> +    <para>
> +     To get the result of the first batch entry the client must call <link
> +     linkend="libpq-PQbatchQueueProcess"><function>PQbatchQueueProcess</function></link>. It must then call

What does 'QueueProcess' mean? Shouldn't it be 'ProcessQueue'? You're
not enquing a process or processing, right?

> +     <function>PQgetResult</function> and handle the results until
> +     <function>PQgetResult</function> returns null (or would return null if
> +     called).

What is that parenthetical referring to?  IIRC we don't provide any
external way to determine PQgetResult would return NULL.

> +    <para>
> +     The client must not assume that work is committed when it
> +     <emphasis>sends</emphasis> a <literal>COMMIT</literal>, only when the
> +     corresponding result is received to confirm the commit is complete.
> +     Because errors arrive asynchronously the application needs to be able to
> +     restart from the last <emphasis>received</emphasis> committed change and
> +     resend work done after that point if something goes wrong.
> +    </para>

That seems like a batch independent thing, right?  If so, maybe make it
a <note>?

> +
> +<synopsis>
> +int PQbatchBegin(PGconn *conn);
...

That function name sounds a bit too much like it'd be relevant for a
single batch, not something that can send many batches. enterBatchMode?

> +int PQbatchEnd(PGconn *conn);
...
""

> +    <varlistentry id="libpq-PQbatchQueueSync">
> +     <term>
> +      <function>PQbatchQueueSync</function>
> +      <function>PQbatchQueueProcess</function>

As said above, I'm not a fan of these, because it sounds like you're
queueing a sync/process.

>  /* ----------------
> @@ -1108,7 +1113,7 @@ pqRowProcessor(PGconn *conn, const char **errmsgp)
>               conn->next_result = conn->result;
>               conn->result = res;
>               /* And mark the result ready to return */
> -             conn->asyncStatus = PGASYNC_READY;
> +             conn->asyncStatus = PGASYNC_READY_MORE;
>       }

Uhm, isn't that an API/ABI breakage issue?

>  /*
> - * Common startup code for PQsendQuery and sibling routines
> + * PQmakePipelinedCommand
> + *   Get a new command queue entry, allocating it if required. Doesn't add it to
> + *   the tail of the queue yet, use PQappendPipelinedCommand once the command has
> + *   been written for that. If a command fails once it's called this, it should
> + *   use PQrecyclePipelinedCommand to put it on the freelist or release it.

"command fails once it's called this"?

> +/*
> + * PQrecyclePipelinedCommand
> + *   Push a command queue entry onto the freelist. It must be a dangling entry
> + *   with null next pointer and not referenced by any other entry's next pointer.
> + */
> +static void
> +PQrecyclePipelinedCommand(PGconn *conn, PGcommandQueueEntry * entry)
> +{
> +     if (entry == NULL)
> +             return;
> +     if (entry->next != NULL)
> +     {
> +             fprintf(stderr, "tried to recycle non-dangling command queue entry");
> +             abort();

Needs a libpq_gettext()?

> +/*
> + * PQbatchEnd
> + *   End batch mode and return to normal command mode.
> + *
> + *   Has no effect unless the client has processed all results
> + *   from all outstanding batches and the connection is idle.
> + *
> + *   Returns true if batch mode ended.
> + */
> +int
> +PQbatchEnd(PGconn *conn)
> +{
> +     if (!conn)
> +             return false;
> +
> +     if (conn->batch_status == PQBATCH_MODE_OFF)
> +             return true;
> +
> +     switch (conn->asyncStatus)
> +     {
> +             case PGASYNC_IDLE:
> +                     printfPQExpBuffer(&conn->errorMessage,
> +                                libpq_gettext_noop("internal error, IDLE in batch mode"));
> +                     break;
> +             case PGASYNC_COPY_IN:
> +             case PGASYNC_COPY_OUT:
> +             case PGASYNC_COPY_BOTH:
> +                     printfPQExpBuffer(&conn->errorMessage,
> +                                libpq_gettext_noop("internal error, COPY in batch mode"));
> +                     break;

Why aren't you returning false here,

> +             case PGASYNC_READY:
> +             case PGASYNC_READY_MORE:
> +             case PGASYNC_BUSY:
> +                     /* can't end batch while busy */
> +                     return false;

but are here?

> +int
> +PQbatchQueueSync(PGconn *conn)
> +{
> +     PGcommandQueueEntry *entry;
> +
> +     if (!conn)
> +             return false;
> +
> +     if (conn->batch_status == PQBATCH_MODE_OFF)
> +             return false;
> +
> +     switch (conn->asyncStatus)
> +     {
> +             case PGASYNC_IDLE:
> +                     printfPQExpBuffer(&conn->errorMessage,
> +                                libpq_gettext_noop("internal error, IDLE in batch mode"));
> +                     break;
> +             case PGASYNC_COPY_IN:
> +             case PGASYNC_COPY_OUT:
> +             case PGASYNC_COPY_BOTH:
> +                     printfPQExpBuffer(&conn->errorMessage,
> +                                libpq_gettext_noop("internal error, COPY in batch mode"));
> +                     break;
> +             case PGASYNC_READY:
> +             case PGASYNC_READY_MORE:
> +             case PGASYNC_BUSY:
> +             case PGASYNC_QUEUED:
> +                     /* can send sync to end this batch of cmds */
> +                     break;
> +     }

Uhm, what is that switch actually achieving? We're not returning an
error code, so ...?

> +     /* Should try to flush immediately if there's room */
> +     PQflush(conn);

"room"?

Also, don't we need to process PQflush's return value?

> +/*
> + * PQbatchQueueProcess
> + *    In batch mode, start processing the next query in the queue.
> + *
> + * Returns true if the next query was popped from the queue and can
> + * be processed by PQconsumeInput, PQgetResult, etc.
> + *
> + * Returns false if the current query isn't done yet, the connection
> + * is not in a batch, or there are no more queries to process.
> + */
> +int
> +PQbatchQueueProcess(PGconn *conn)
> +{
> +     PGcommandQueueEntry *next_query;
> +
> +     if (!conn)
> +             return false;
> +
> +     if (conn->batch_status == PQBATCH_MODE_OFF)
> +             return false;
> +
> +     switch (conn->asyncStatus)
> +     {
> +             case PGASYNC_COPY_IN:
> +             case PGASYNC_COPY_OUT:
> +             case PGASYNC_COPY_BOTH:
> +                     printfPQExpBuffer(&conn->errorMessage,
> +                                libpq_gettext_noop("internal error, COPY in batch mode"));
> +                     break;
> +             case PGASYNC_READY:
> +             case PGASYNC_READY_MORE:
> +             case PGASYNC_BUSY:
> +                     /* client still has to process current query or results */
> +                     return false;
> +                     break;
> +             case PGASYNC_IDLE:
> +                     printfPQExpBuffer(&conn->errorMessage,
> +                                libpq_gettext_noop("internal error, IDLE in batch mode"));
> +                     break;
> +             case PGASYNC_QUEUED:
> +                     /* next query please */
> +                     break;
> +     }

Once more, I'm very unconvinced by the switch. Unless you do anything
with the errors, this seems pointless.

> +     if (conn->batch_status == PQBATCH_MODE_ABORTED && conn->queryclass != PGQUERY_SYNC)
> +     {
> +             /*
> +              * In an aborted batch we don't get anything from the server for each
> +              * result; we're just discarding input until we get to the next sync
> +              * from the server. The client needs to know its queries got aborted
> +              * so we create a fake PGresult to return immediately from
> +              * PQgetResult.
> +              */
> +             conn->result = PQmakeEmptyPGresult(conn,
> +                                                                                PGRES_BATCH_ABORTED);
> +             if (!conn->result)
> +             {
> +                     printfPQExpBuffer(&conn->errorMessage,
> +                                                       libpq_gettext("out of memory"));
> +                     pqSaveErrorResult(conn);
> +             }
> +             conn->asyncStatus = PGASYNC_READY;

So we still return true in the OOM case?