Re: PATCH: Batch/pipelining support for libpq - Mailing list pgsql-hackers

On 2017-04-03 14:10:47 +1000, Vaishnavi Prabakaran wrote:
> > The CF has been extended until April 7 but time is still growing short.
> > Please respond with a new patch by 2017-04-04 00:00 AoE (UTC-12) or this
> > submission will be marked "Returned with Feedback".
> >
> >
> Thanks for the information, attached the latest patch resolving one
> compilation warning. And, please discard the test patch as it will be
> re-implemented later separately.

Hm.  If the tests aren't ready yet, it seems we'll have to move this to
the next CF.


> + <sect1 id="libpq-batch-mode">
> +  <title>Batch mode and query pipelining</title>
> +
> +  <indexterm zone="libpq-batch-mode">
> +   <primary>libpq</primary>
> +   <secondary>batch mode</secondary>
> +  </indexterm>
> +
> +  <indexterm zone="libpq-batch-mode">
> +   <primary>libpq</primary>
> +   <secondary>pipelining</secondary>
> +  </indexterm>
> +
> +  <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.


Have you checked how this API works with PQsetSingleRowMode()?

> +    <para>
> +     To enter single-row mode, call <function>PQsetSingleRowMode</function> immediately after a
> +     successful call of <function>PQbatchQueueProcess</function>. This mode selection is effective 
> +     only for the query currently being processed. For more information on the use of <function>PQsetSingleRowMode
> +     </function>, refer to <xref linkend="libpq-single-row-mode">.

Hah ;).


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


> +     <listitem>
> +      <para>
> +      Causes a connection to enter batch mode if it is currently idle or
> +      already in batch mode.
> +
> +<synopsis>
> +int PQbatchBegin(PGconn *conn);
> +</synopsis>
> +
> +        </para>
> +        <para>
> +          Returns 1 for success. Returns 0 and has no 
> +          effect if the connection is not currently idle, i.e. it has a result 
> +          ready, is waiting for more input from the server, etc. This function 
> +          does not actually send anything to the server, it just changes the 
> +          <application>libpq</application> connection state.
> +
> +        </para>
> +     </listitem>
> +    </varlistentry>

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?

> +    <varlistentry id="libpq-PQbatchEnd">
> +     <term>
> +      <function>PQbatchEnd</function>
> +      <indexterm>
> +       <primary>PQbatchEnd</primary>
> +      </indexterm>
> +     </term>
> +
> +     <listitem>
> +      <para>
> +      Causes a connection to exit batch mode if it is currently in batch mode
> +      with an empty queue and no pending results.
> +<synopsis>
> +int PQbatchEnd(PGconn *conn);
> +</synopsis>
> +        </para>
> +        <para>Returns 1 for success.
> +      Returns 1 and takes no action if not in batch mode. If the connection has
> +      pending batch items in the queue for reading with
> +      <function>PQbatchQueueProcess</function>, the current statement isn't finished
> +      processing or there are results pending for collection with
> +      <function>PQgetResult</function>, returns 0 and does nothing.
> +
> +      </para>
> +     </listitem>
> +    </varlistentry>

""


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

> +        case PGASYNC_QUEUED:
> +            break;
> +    }
> +


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



Greetings,

Andres Freund