Re: PATCH: Batch/pipelining support for libpq

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