Re: PATCH: Batch/pipelining support for libpq

Andres Freund <andres(at)anarazel(dot)de> wrote:

> > +
> > + <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.

Re-phrased the sentence as "libpq supports queries to be queued up in
batches and pipeline them to the server, where it will be executed as a
batch" . Pipelining queries allows applications to avoid a client/server
round-trip after each query to get the results before issuing the next
query.

>
> > + <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/
>
>
Corrected.

>
> > + <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/?
>
>
Corrected.

>
> > 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.
>
>
Removed the parenthesis as the line before gives enough warning.

>
> > + <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.
>
>
Corrected by replacing "concurrently" with "And to get results"

>
> > + <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.
>
>
True, and in the documentation , how to interleave result processing and
query dispatch is given.

>
> > + <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?
>
>
Regardless of transaction boundaries, failed command will make the entire
batch aborted and no commands will be processed in server until Batch sync
is sent. And, if the command inside the transaction fails, then the batch
will be aborted, but transaction will still be open in the server. Client
explicitly needs to close the transaction. This is explained further down
in the documentation section "Error handling" , paragraph starting with "If
the batch used an implicit transaction.." and "Note" in that section gives
warning too.

>
> > + <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?
>
>
Corrected.

>
> > + <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?
>
>
Changed to PQbatchProcessQueue.

>
> > + <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.
>
>
True, it is just application's extra knowledge. Removed the parenthesis.

> > + <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>?
>
>
Yes, made it as 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);
> ...
> ""
>
>
Changed the function names to PQenterBatchMode and PQexitBatchMode.

>
> > + <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.
>
>
Changed it to PQbatchSyncQueue.

>
> > /* ----------------
> > @@ -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?
>
>
I think no, asynstatus is in libpq-int.h which is meant to be used only by
frontend libpq, applications should use libpq-fe.h . Also, we don't have
any API to return the async status of connection to application. So, I
think it will not break any existing applications.

>
> > /*
> > - * 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"?
>
>
Corrected.- "Command sending fails"

>
> > +/*
> > + * 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()?
>
>
>
Corrected.

>
> > +/*
> > + * 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,

Using copy command in batch mode is the error case, so instead of giving
opportunity to the application to finish the error scenario, we set the
error message and allows it to quit the batch mode.

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

Application gets a chance to finish the result processing and exit the
batch mode.

> > +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 ...?

These errors are marked here, so that the application later when reading
the results for batch queries, will have more information about what went
wrong. This is just an additional information to user only, but doesn't
stop the error scenario.

>
>
> > + /* Should try to flush immediately if there's room */
> > + PQflush(conn);
>
> "room"?
>
> Also, don't we need to process PQflush's return value?
>
>
Corrected both .

>
> > +/*
> > + * 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.

Same answer as above switch question.

>
>
> > + 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?
>
>
Corrected to return false, as further calls to this function will also
fail. And , application as well can check for this failure reason from
PQgetResult.

Regarding test patch, I have corrected the test suite after David Steele's
comments.
Also, I would like to mention that a companion patch was submitted by David
Steele up-thread.

Attached the latest code and test patch.

Thanks & Regards,
Vaishnavi,
Fujitsu Australia.