Re: PATCH: Batch/pipelining support for libpq

On Wed, Aug 23, 2017 at 7:40 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:

>
>
>
> > Am failing to see the benefit in allowing user to set
> > PQBatchAutoFlush(true|false) property? Is it really needed?
>
> I'm inclined not to introduce that for now. If somebody comes up with a
> convincing usecase and numbers, we can add it later. Libpq API is set in
> stone, so I'd rather not introduce unnecessary stuff...
>
>
Thanks for reviewing the patch and yes ok.

>
>
> > + <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
> > + can sometimes offer considerable performance improvements.
> > + </para>
>
> That's not necessarily true, is it? Unless you count always doing
> batches of exactly size 1.
>

Client application complexity is increased in batch mode,because
application needs to remember the query queue status. Results processing
can be done at anytime, so the application needs to know till what query,
the results are consumed.

> + <para>
> > + Use batches when your application does lots of small
> > + <literal>INSERT</literal>, <literal>UPDATE</literal> and
> > + <literal>DELETE</literal> operations that can't easily be
> transformed into
> > + operations on sets or into a
> > + <link linkend="libpq-copy"><literal>COPY</literal></link>
> operation.
> > + </para>
>
> Aren't SELECTs also a major beneficiarry of this?
>

Hmm, though SELECTs also benefit from batch mode, doing multiple selects in
batch mode will fill up the memory rapidly and might not be as beneficial
as other operations listed.

> > + <para>
> > + Batching is less useful when information from one operation is
> required by the
> > + client before it knows enough to send the next operation.
>
> s/less/not/
>
>
Corrected.

>
> > + <note>
> > + <para>
> > + The batch API was introduced in PostgreSQL 10.0, but clients using
> PostgresSQL 10.0 version of libpq can
> > + use batches on server versions 8.4 and newer. Batching works on
> any server
> > + that supports the v3 extended query protocol.
> > + </para>
> > + </note>
>
> Where's the 8.4 coming from?
>
>

I guess it is 7.4 where "PQsendQueryParams" is introduced, and not 8.4.
Corrected.

> + <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>
>
> Mention that nonblocking only actually helps if send/recv is done as
> required, and can essentially require unbound memory? We probably
> should either document or implement some smarts about when to signal
> read/write readyness. Otherwise we e.g. might be receiving tons of
> result data without having sent the next query - or the other way round.
>
>

Added a statement for caution in documentation and again this is one of the
reason why SELECT query is not so beneficial in batch mode.

> Maybe note that multiple batches can be "in flight"?
> I.e. PQbatchSyncQueue() is about error handling, nothing else? Don't
> have a great idea, but we might want to rename...
>
>
This function not only does error handling, but also sends the "Sync"
message to backend. In batch mode, "Sync" message is not sent with every
query but will
be sent only via this function to mark the end of implicit transaction.
Renamed it to PQbatchCommitQueue. Kindly let me know if you think of any
other better name.

> > + <note>
> > + <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>
> > + </note>
>
> This seems fairly independent of batching.
>
>
Yes and the reason why is it explicitly specified for batch mode is that if
more than one explicit transactions are used in Single batch, then failure
of one transaction will lead to skipping the consequent transactions until
the end of current batch is reached. This behavior is specific to batch
mode, so adding a precautionary note here is needed I think.

> > + </sect3>
> > +
> > + <sect3 id="libpq-batch-interleave">
> > + <title>Interleaving result processing and query dispatch</title>
> > +
> > + <para>
> > + To avoid deadlocks on large batches the client should be
> structured around
> > + a nonblocking I/O loop using a function like
> <function>select</function>,
> > + <function>poll</function>, <function>epoll</function>,
> > + <function>WaitForMultipleObjectEx</function>, etc.
> > + </para>
> > +
> > + <para>
> > + The client application should generally maintain a queue of work
> still to
> > + be dispatched and a queue of work that has been dispatched but not
> yet had
> > + its results processed.
>
> Hm. Why? If queries are just issued, no such queue is required?
>

This is essentially telling that client application should remember the
order of queries sent , so that result processing will be based on what
results are expected. For e.g, if the order of queries are 1)SELECT,
2)INSERT, then in result processing, "PGRES_TUPLES_OK" and later
"PGRES_COMMAND_OK" checks are required. Above mentioned queue of work means
the intelligent to remember the queue of queries.

> > When the socket is writable it should dispatch more
> > + work. When the socket is readable it should read results and
> process them,
> > + matching them up to the next entry in its expected results queue.
> Batches
> > + should be scoped to logical units of work, usually (but not
> always) one
> > + transaction per batch. There's no need to exit batch mode and
> re-enter it
> > + between batches or to wait for one batch to finish before sending
> the next.
> > + </para>
>
> This really needs to take memory usage into account.
>
>
Modified the para to include that frequency of result processing should be
based on available memory.

> > +<synopsis>
> > +int PQenterBatchMode(PGconn *conn);
> > +</synopsis>
> > +<synopsis>
> > +int PQexitBatchMode(PGconn *conn);
> > +</synopsis>
> > + </para>
> > + <para>Returns 1 for success.
> > + Returns 1 and takes no action if not in batch mode. If the
> connection has
>
> "returns 1"?
>

Modified the code to return 1/0.

>
>
> > + <varlistentry id="libpq-PQbatchSyncQueue">
> > + <term>
> > + <function>PQbatchSyncQueue</function>
> > + <indexterm>
> > + <primary>PQbatchSyncQueue</primary>
> > + </indexterm>
> > + </term>
>
> I wonder why this isn't framed as PQbatchIssue/Send/...()? Syncing seems
> to mostly make sense from a protocol POV.
>
>
Renamed to PQbatchCommitQueue.

> > +<synopsis>
> > +int PQbatchQueueCount(PGconn *conn);
> > +</synopsis>
> > +
>
> Given that apps are supposed to track this, I'm not sure why we have
> this?
>
>
Removed this function considering your another comment about O(N) as well.

> > +static void
> > +PQrecyclePipelinedCommand(PGconn *conn, PGcommandQueueEntry * entry)
> > +{
> > + if (entry == NULL)
> > + return;
> > + if (entry->next != NULL)
> > + {
> > + fprintf(stderr, libpq_gettext("tried to recycle
> non-dangling command queue entry"));
> > + abort();
>
> Don't think we use abort() in libpq like that. There's some Assert()s
> tho.
>
>

For out-of-memory cases, we do abort(), and here abort will happen only
during some memory corruption. So, I think it is ok to abort here. Please
let me know if you think otherwise.

>
> > static bool
> > PQsendQueryStart(PGconn *conn)
> .....

>
Weirdly indented.
>
>
Corrected.

> > +/*
> > + * PQbatchBegin
>
> Mismatched w/ actual function name.
>
>
Corrected.

>
> > + * Put an idle connection in batch mode. Commands submitted after this
> > + * can be pipelined on the connection, there's no requirement to wait
> for
> > + * one to finish before the next is dispatched.
> > + *
> > + * Queuing of new query or syncing during COPY is not allowed.
>
> +"a"?
>

Hmm, Can you explain the question please. I don't understand.

>
> > +/*
> > + * PQbatchEnd
>
> wrong name.
>

Corrected.

>
> > + * 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.
>
> That seems wrong - will lead to hard to diagnose errors.

I have now added a error message when the batch end is failed to ease the
error diagnose. And, this behavior is documented. So the application should
not assume that the batch mode ended without checking the return value.

> > + * Returns true if batch mode ended.
> > + */
> > +int
> > +PQexitBatchMode(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;
>
> So we'll still check the queue in this case, that's a bit weird?
>

Removed above cases as adding error here is not of much help as well.

>
> > +/*
> > + * PQbatchQueueSync
>
> Out of sync.
>

Corrected.

>
> > + * End a batch submission by sending a protocol sync. The connection
> will
> > + * remain in batch mode and unavailable for new non-batch commands
> until all
> > + * results from the batch are processed by the client.
>
> "unavailable for new non-batch commands" - that's hard to follow, and
> seems pretty redundant with PQendBatchMode (or however it's called).
>
>
Modified documentation to be more precise and difference between
PQexitBatchMode and this one is that to exit batch, application should have
consumed all the results pending in buffer and post exit batch, an
application can call synchronous command execution functions. Whereas with
batch sync/commit, application cannot issue synchronous commands but can
issue asynchronous commands without reading all the results pending in
buffer. This function basically marks the end of implicit transaction.

> > + * It's legal to start submitting another batch immediately, without
> waiting
> > + * for the results of the current batch. There's no need to end batch
> mode
> > + * and start it again.
> > + *
> > + * If a command in a batch fails, every subsequent command up to and
> including
> > + * the PQbatchQueueSync command result gets set to
> PGRES_BATCH_ABORTED state. If the
> > + * whole batch is processed without error, a PGresult with
> PGRES_BATCH_END is
> > + * produced.
>
> Hm, should probably mention that that's only true for commands since the
> last PQbatchQueueSync?
>
>

"Batch" means the set of commands between PQbatchCommitQueue(newly renamed
name) calls except first batch which count from beginning to
PQbatchCommitQueue. And in documentation it is mentioned that "The result
for <function>PQbatchSyncQueue</function> is reported as
<literal>PGRES_BATCH_END</literal> to signal the end of the aborted batch
and resumption of normal result processing."

>
> > +/*
> > + * PQbatchQueueProcess
>
> Out of sync.
>

Corrected .

> > + * 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.
>
> Last complaint about this - think this forgiving mode is a mistake.
>
>
Now added an error message using printfPQExpBuffer(). I think aborting here
is not really needed. Please let me know if you prefer to abort() rather
than this solution.

>
> > + */
> > +int
> > +PQbatchProcessQueue(PGconn *conn)
> > +{
>
> > + /* This command's results will come in immediately.
> > + * Initialize async result-accumulation state */
> > + pqClearAsyncResult(conn);
>
> I'm not following?
>
>
First line of comment is removed. It is copy/paste error.

>
> > /*
> > * PQgetResult
> > @@ -1749,10 +2228,32 @@ PQgetResult(PGconn *conn)
>
> > + if (conn->batch_status != PQBATCH_MODE_OFF)
> > + {
> > + /*
> > + * batched queries aren't followed by a
> Sync to put us back in
> > + * PGASYNC_IDLE state, and when we do get
> a sync we could
> > + * still have another batch coming after
> this one.
>
> This needs rephrasing.
>
>

Rephrased as "In batch mode, query execution state cannot be IDLE as there
can be other queries or results waiting in the queue ..." . Hope this is
simple and enough.

> > + * The connection isn't idle since we
> can't submit new
> > + * nonbatched commands. It isn't also busy
> since the current
> > + * command is done and we need to process
> a new one.
> > + */
> > + conn->asyncStatus = PGASYNC_QUEUED;
>
> Not sure I like the name.
>
>
Changed the name to PGASYNC_BATCH to make it more align to batch mode as
this status will not be set anywhere in non-batch mode.

>
> > + if (conn->asyncStatus == PGASYNC_QUEUED || conn->batch_status !=
> PQBATCH_MODE_OFF)
> > + {
> > + printfPQExpBuffer(&conn->errorMessage,
> > +
> libpq_gettext("Synchronous command execution functions are not allowed in
> batch mode\n"));
> > + return false;
> > + }
>
> Why do we need the PGASYNC_QUEUED test here?
>

Yes, we don't need this check here. It was removed in PQfn and similarly it
is not needed here and in pqParseInput2 too.

> > +static int
> > +pqBatchFlush(PGconn *conn)
> > +{
> > + if ((conn->batch_status == PQBATCH_MODE_OFF)||(conn->
> outCount>=65536))
> > + return(pqFlush(conn));
> > + return 0; /* Just to keep compiler quiet */
> > +}
>
> This should be defined in a macro or such, rather than hardcoded.
>
>
Added macro "OUTBUFFER_THRESHOLD"

>
> Falling over now. This seems like enough feedback for a bit of work
> anyway.
>

Thanks once again for reviewing the patch. Attached the patch with your
review comments incorporation.

Thanks & Regards,
Vaishnavi,
Fujitsu Australia.